On 1/5/22 03:41, Roman Gushchin wrote:
> On Tue, Jan 04, 2022 at 01:10:37AM +0100, Vlastimil Babka wrote:
>> page->memcg_data is used with MEMCG_DATA_OBJCGS flag only for slab pages
>> so convert all the related infrastructure to struct slab. Also use
>> struct folio instead of struct page when resolving object pointers.
>>
>> This is not just mechanistic changing of types and names. Now in
>> mem_cgroup_from_obj() we use folio_test_slab() to decide if we interpret
>> the folio as a real slab instead of a large kmalloc, instead of relying
>> on MEMCG_DATA_OBJCGS bit that used to be checked in page_objcgs_check().
>> Similarly in memcg_slab_free_hook() where we can encounter
>> kmalloc_large() pages (here the folio slab flag check is implied by
>> virt_to_slab()). As a result, page_objcgs_check() can be dropped instead
>> of converted.
>>
>> To avoid include cycles, move the inline definition of slab_objcgs()
>> from memcontrol.h to mm/slab.h.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
>> Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>> Cc: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
>> Cc: <cgroups@xxxxxxxxxxxxxxx>
>> /*
>> * Slab objects are accounted individually, not per-page.
>> * Memcg membership data for each individual object is saved in
>> * the page->obj_cgroups.
> ^^^^^^^^^^^^^^^^^
> slab->memcg_data
Good catch, fixed.
>> */
>> - if (page_objcgs_check(page)) {
>> - struct obj_cgroup *objcg;
>> + if (folio_test_slab(folio)) {
>> + struct obj_cgroup **objcgs;
>> + struct slab *slab;
>> unsigned int off;
>>
>> - off = obj_to_index(page->slab_cache, page_slab(page), p);
>> - objcg = page_objcgs(page)[off];
>> - if (objcg)
>> - return obj_cgroup_memcg(objcg);
>> + slab = folio_slab(folio);
>> + objcgs = slab_objcgs(slab);
>> + if (!objcgs)
>> + return NULL;
>> +
>> + off = obj_to_index(slab->slab_cache, slab, p);
>> + if (objcgs[off])
>> + return obj_cgroup_memcg(objcgs[off]);
>>
>> return NULL;
>> }
>
> There is a comment below, which needs some changes:
> /*
> * page_memcg_check() is used here, because page_has_obj_cgroups()
> * check above could fail because the object cgroups vector wasn't set
> * at that moment, but it can be set concurrently.
> * page_memcg_check(page) will guarantee that a proper memory
> * cgroup pointer or NULL will be returned.
> */
>
> In reality the folio's slab flag can be cleared before releasing the objcgs \
> vector. It seems that there is no such possibility at setting the flag,
> it's always set before allocating and assigning the objcg vector.
You're right. I'm changing it to:
* page_memcg_check() is used here, because in theory we can encounter
* a folio where the slab flag has been cleared already, but
* slab->memcg_data has not been freed yet
* page_memcg_check(page) will guarantee that a proper memory
* cgroup pointer or NULL will be returned.
I wrote "in theory" because AFAICS it implies a race as we would have to be
freeing a slab and at the same time query an object address. We probably
could have used the non-check version, but at this point I don't want to
make any functional changes besides these comment fixes.
I assume your patch on top would cover it?
>> @@ -2896,7 +2901,7 @@ struct mem_cgroup *mem_cgroup_from_obj(void *p)
>> * page_memcg_check(page) will guarantee that a proper memory
>> * cgroup pointer or NULL will be returned.
>> */
>> - return page_memcg_check(page);
>> + return page_memcg_check(folio_page(folio, 0));
>> }
>>
>> __always_inline struct obj_cgroup *get_obj_cgroup_from_current(void)
>> diff --git a/mm/slab.h b/mm/slab.h
>> index bca9181e96d7..36e0022d8267 100644
>> --- a/mm/slab.h
>> +++ b/mm/slab.h
>> @@ -412,15 +412,36 @@ static inline bool kmem_cache_debug_flags(struct kmem_cache *s, slab_flags_t fla
>> }
>>
>> #ifdef CONFIG_MEMCG_KMEM
>> -int memcg_alloc_page_obj_cgroups(struct page *page, struct kmem_cache *s,
>> - gfp_t gfp, bool new_page);
>> +/*
>> + * slab_objcgs - get the object cgroups vector associated with a slab
>> + * @slab: a pointer to the slab struct
>> + *
>> + * Returns a pointer to the object cgroups vector associated with the slab,
>> + * or NULL. This function assumes that the slab is known to have an
>> + * associated object cgroups vector. It's not safe to call this function
>> + * against slabs with underlying pages, which might have an associated memory
>> + * cgroup: e.g. kernel stack pages.
>
> Hm, is it still true? I don't think so. It must be safe to call it for any
> slab now.
Right, forgot to update after removing the _check variant.
Changing to:
* Returns a pointer to the object cgroups vector associated with the slab,
* or NULL if no such vector has been associated yet.
> The rest looks good to me, please feel free to add
> Reviewed-by: Roman Gushchin <guro@xxxxxx>
> after fixing these comments.
Thanks!
> Thanks!
