mm: memcg/slab: Uncharge during kmem_cache_free_bulk()
Subsystems (2)
PATCH 1/1 mm: memcg/slab: Uncharge during kmem_cache_free_bulk()
Object cgroup charging is done for all the objects during allocation, but during freeing, uncharging ends up happening for only one object in the case of bulk allocation/freeing.
Fix this by having a separate call to uncharge all the objects from kmem_cache_free_bulk() and by modifying memcg_slab_free_hook() to take care of bulk uncharging.
--- mm/slab.c | 2 +- mm/slab.h | 42 +++++++++++++++++++++++++++--------------- mm/slub.c | 3 ++- 3 files changed, 30 insertions(+), 17 deletions(-)
mm/slab.c +1 -1
@@ -3440,7 +3440,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,memset (objp,0 , cachep->object_size);kmemleak_free_recursive (objp, cachep->flags); objp =cache_free_debugcheck (cachep, objp, caller); -memcg_slab_free_hook (cachep,virt_to_head_page (objp), objp); +memcg_slab_free_hook (cachep, &objp,1 );/* * Skip calling cache_free_alien() when the platform is not numa.
mm/slab.h +27 -15
65 lines not on this page · raw message
mm/slub.c +2 -1
@@ -3091,7 +3091,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,struct kmem_cache_cpu *c;unsigned long tid; -memcg_slab_free_hook (s, page, head); +memcg_slab_free_hook (s, &head,1 ); redo:/* * Determine the currently cpus per cpu slab.
@@ -3253,6 +3253,7 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)if (WARN_ON (!size))return ; +memcg_slab_free_hook (s, p, size); do { struct detached_freelist df;
Signature · 1 line
2.26.2
On Fri, Oct 09, 2020 at 11:34:23AM +0530, Bharata B Rao wrote:
Hi Bharata,
Quoted 3 lines
Object cgroup charging is done for all the objects during allocation, but during freeing, uncharging ends up happening for only one object in the case of bulk allocation/freeing.
Yes, it's definitely a problem. Thank you for catching it!
I'm curious, did you find it in the wild or by looking into the code?
Quoted 6 lines
Fix this by having a separate call to uncharge all the objects from
kmem_cache_free_bulk()and by modifyingmemcg_slab_free_hook()to take care of bulk uncharging.Signed-off-by Bharata B Rao <[email protected]>
Please, add:
Quoted 95 lines
--- mm/slab.c | 2 +- mm/slab.h | 42 +++++++++++++++++++++++++++--------------- mm/slub.c | 3 ++- 3 files changed, 30 insertions(+), 17 deletions(-)mm/slab.c +1
-1@@ -3440,7 +3440,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,memset (objp,0 , cachep->object_size);kmemleak_free_recursive (objp, cachep->flags); objp =cache_free_debugcheck (cachep, objp, caller); -memcg_slab_free_hook (cachep,virt_to_head_page (objp), objp); +memcg_slab_free_hook (cachep, &objp,1 );/* * Skip calling cache_free_alien() when the platform is not numa. mm/slab.h +27
-15@@ -345,30 +345,42 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,obj_cgroup_put (objcg); } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s_orig, +void **p,int objects) { +struct kmem_cache *s;struct obj_cgroup *objcg; +struct page *page;unsigned int off; +int i;if (!memcg_kmem_enabled ())return ; -if (!page_has_obj_cgroups (page)) -return ; +for (i =0 ; i < objects; i++) { +if (unlikely (!p[i])) +continue ; - off =obj_to_index (s, page, p); - objcg =page_obj_cgroups (page)[off]; -page_obj_cgroups (page)[off] =NULL ; + page =virt_to_head_page (p[i]); +if (!page_has_obj_cgroups (page)) +continue ; -if (!objcg) -return ; +if (!s_orig) + s = page->slab_cache; +else + s = s_orig; -obj_cgroup_uncharge (objcg,obj_full_size (s)); -mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), - -obj_full_size (s)); + off =obj_to_index (s, page, p[i]); + objcg =page_obj_cgroups (page)[off]; +if (!objcg) +continue ; -obj_cgroup_put (objcg); +page_obj_cgroups (page)[off] =NULL ; +obj_cgroup_uncharge (objcg,obj_full_size (s)); +mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), + -obj_full_size (s)); +obj_cgroup_put (objcg); + } }#else /* CONFIG_MEMCG_KMEM */ @@ -406,8 +418,8 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s, { } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s, +void **p,int objects) { }#endif /* CONFIG_MEMCG_KMEM */ mm/slub.c +0
-0
mm/slub.c static __always_inline void do_slab_free(struct kmem_cache *s, · line 3094
Quoted 5 lines
mm/slub.c +1
-1@@ -3091,7 +3091,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,struct kmem_cache_cpu *c;unsigned long tid; -memcg_slab_free_hook (s, page, head); +memcg_slab_free_hook (s, &head,1 );
Hm, I wonder if it's better to teach do_slab_free() to handle the (cnt > 1) case correctly?
mm/slub.c void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) · line 3256
Quoted 8 lines
mm/slub.c +1
-0redo:/* * Determine the currently cpus per cpu slab. @@ -3253,6 +3253,7 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)if (WARN_ON (!size))return ; +memcg_slab_free_hook (s, p, size);
Then you don't need this change.
Otherwise memcg_slab_free_hook() can be called twice for the same object. It's ok from accounting correctness perspective, because the first call will zero out the objcg pointer, but still much better to be avoided.
Thanks!
Quoted 11 lines · Roman Gushchin
On Fri, Oct 09, 2020 at 11:34:23AM +0530, Bharata B Rao wrote:
Hi Bharata,
Quoted 3 lines
Object cgroup charging is done for all the objects during allocation, but during freeing, uncharging ends up happening for only one object in the case of bulk allocation/freeing.
Yes, it's definitely a problem. Thank you for catching it!
I'm curious, did you find it in the wild or by looking into the code?
Found by looking at the code.
Quoted 111 lines
Quoted 7 lines
Fix this by having a separate call to uncharge all the objects from
kmem_cache_free_bulk()and by modifyingmemcg_slab_free_hook()to take care of bulk uncharging.Signed-off-by Bharata B Rao <[email protected]>Please, add:
Fixes 964d4bd370d5 ("mm: memcg/slab: save obj_cgroup for non-root slab objects")Quoted 98 lines
--- mm/slab.c | 2 +- mm/slab.h | 42 +++++++++++++++++++++++++++--------------- mm/slub.c | 3 ++- 3 files changed, 30 insertions(+), 17 deletions(-)mm/slab.c +1
-1@@ -3440,7 +3440,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,memset (objp,0 , cachep->object_size);kmemleak_free_recursive (objp, cachep->flags); objp =cache_free_debugcheck (cachep, objp, caller); -memcg_slab_free_hook (cachep,virt_to_head_page (objp), objp); +memcg_slab_free_hook (cachep, &objp,1 );/* * Skip calling cache_free_alien() when the platform is not numa. mm/slab.h +27
-15@@ -345,30 +345,42 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,obj_cgroup_put (objcg); } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s_orig, +void **p,int objects) { +struct kmem_cache *s;struct obj_cgroup *objcg; +struct page *page;unsigned int off; +int i;if (!memcg_kmem_enabled ())return ; -if (!page_has_obj_cgroups (page)) -return ; +for (i =0 ; i < objects; i++) { +if (unlikely (!p[i])) +continue ; - off =obj_to_index (s, page, p); - objcg =page_obj_cgroups (page)[off]; -page_obj_cgroups (page)[off] =NULL ; + page =virt_to_head_page (p[i]); +if (!page_has_obj_cgroups (page)) +continue ; -if (!objcg) -return ; +if (!s_orig) + s = page->slab_cache; +else + s = s_orig; -obj_cgroup_uncharge (objcg,obj_full_size (s)); -mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), - -obj_full_size (s)); + off =obj_to_index (s, page, p[i]); + objcg =page_obj_cgroups (page)[off]; +if (!objcg) +continue ; -obj_cgroup_put (objcg); +page_obj_cgroups (page)[off] =NULL ; +obj_cgroup_uncharge (objcg,obj_full_size (s)); +mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), + -obj_full_size (s)); +obj_cgroup_put (objcg); + } }#else /* CONFIG_MEMCG_KMEM */ @@ -406,8 +418,8 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s, { } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s, +void **p,int objects) { }#endif /* CONFIG_MEMCG_KMEM */ mm/slub.c +0
-0@@ -3091,7 +3091,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,struct kmem_cache_cpu *c;unsigned long tid;
Quoted 4 lines
mm/slub.c static __always_inline void do_slab_free(struct kmem_cache *s, · line 3094
Quoted 2 lines
mm/slub.c +1
-1-memcg_slab_free_hook (s, page, head); +memcg_slab_free_hook (s, &head,1 );Hm, I wonder if it's better to teach
do_slab_free()to handle the (cnt > 1) case correctly?
Possible, but we will have to walk the detached freelist there while here it is much easier to just walk the array of objects?
Quoted 8 lines
Quoted 8 lines
mm/slub.c +0
-0redo:/* * Determine the currently cpus per cpu slab. @@ -3253,6 +3253,7 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)if (WARN_ON (!size))return ;
Quoted 7 lines
mm/slub.c void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) · line 3256
Quoted 1 line
mm/slub.c +1
-0+memcg_slab_free_hook (s, p, size);Then you don't need this change.
Otherwise
memcg_slab_free_hook()can be called twice for the same object. It's ok from accounting correctness perspective, because the first call will zero out the objcg pointer, but still much better to be avoided.
Yes, for that one object there will be one additional uncharge call, but as you note it is handled by the !objcg check.
Regards,
Bharata.
Roman Gushchin
Acked-byQuoted 146 lines · Bharata B Rao
Quoted 11 lines · Roman Gushchin
On Fri, Oct 09, 2020 at 11:34:23AM +0530, Bharata B Rao wrote:
Hi Bharata,
Quoted 3 lines
Object cgroup charging is done for all the objects during allocation, but during freeing, uncharging ends up happening for only one object in the case of bulk allocation/freeing.
Yes, it's definitely a problem. Thank you for catching it!
I'm curious, did you find it in the wild or by looking into the code?
Found by looking at the code.
Quoted 111 lines
Quoted 7 lines
Fix this by having a separate call to uncharge all the objects from
kmem_cache_free_bulk()and by modifyingmemcg_slab_free_hook()to take care of bulk uncharging.Signed-off-by Bharata B Rao <[email protected]>Please, add:
Fixes 964d4bd370d5 ("mm: memcg/slab: save obj_cgroup for non-root slab objects")Quoted 98 lines
--- mm/slab.c | 2 +- mm/slab.h | 42 +++++++++++++++++++++++++++--------------- mm/slub.c | 3 ++- 3 files changed, 30 insertions(+), 17 deletions(-)mm/slab.c +1
-1@@ -3440,7 +3440,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,memset (objp,0 , cachep->object_size);kmemleak_free_recursive (objp, cachep->flags); objp =cache_free_debugcheck (cachep, objp, caller); -memcg_slab_free_hook (cachep,virt_to_head_page (objp), objp); +memcg_slab_free_hook (cachep, &objp,1 );/* * Skip calling cache_free_alien() when the platform is not numa. mm/slab.h +27
-15@@ -345,30 +345,42 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s,obj_cgroup_put (objcg); } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s_orig, +void **p,int objects) { +struct kmem_cache *s;struct obj_cgroup *objcg; +struct page *page;unsigned int off; +int i;if (!memcg_kmem_enabled ())return ; -if (!page_has_obj_cgroups (page)) -return ; +for (i =0 ; i < objects; i++) { +if (unlikely (!p[i])) +continue ; - off =obj_to_index (s, page, p); - objcg =page_obj_cgroups (page)[off]; -page_obj_cgroups (page)[off] =NULL ; + page =virt_to_head_page (p[i]); +if (!page_has_obj_cgroups (page)) +continue ; -if (!objcg) -return ; +if (!s_orig) + s = page->slab_cache; +else + s = s_orig; -obj_cgroup_uncharge (objcg,obj_full_size (s)); -mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), - -obj_full_size (s)); + off =obj_to_index (s, page, p[i]); + objcg =page_obj_cgroups (page)[off]; +if (!objcg) +continue ; -obj_cgroup_put (objcg); +page_obj_cgroups (page)[off] =NULL ; +obj_cgroup_uncharge (objcg,obj_full_size (s)); +mod_objcg_state (objcg,page_pgdat (page),cache_vmstat_idx (s), + -obj_full_size (s)); +obj_cgroup_put (objcg); + } }#else /* CONFIG_MEMCG_KMEM */ @@ -406,8 +418,8 @@ static inline void memcg_slab_post_alloc_hook(struct kmem_cache *s, { } -static inline void memcg_slab_free_hook (struct kmem_cache *s,struct page *page, -void *p) +static inline void memcg_slab_free_hook (struct kmem_cache *s, +void **p,int objects) { }#endif /* CONFIG_MEMCG_KMEM */ mm/slub.c +0
-0@@ -3091,7 +3091,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,struct kmem_cache_cpu *c;unsigned long tid;Quoted 4 lines
mm/slub.c static __always_inline void do_slab_free(struct kmem_cache *s, · line 3094
Quoted 2 lines
mm/slub.c +1
-1-memcg_slab_free_hook (s, page, head); +memcg_slab_free_hook (s, &head,1 );Hm, I wonder if it's better to teach
do_slab_free()to handle the (cnt > 1) case correctly?Possible, but we will have to walk the detached freelist there while here it is much easier to just walk the array of objects?
Quoted 12 lines
mm/slub.c void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p) · line 3256
Quoted 9 lines
mm/slub.c +1
-0redo:/* * Determine the currently cpus per cpu slab. @@ -3253,6 +3253,7 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)if (WARN_ON (!size))return ; +memcg_slab_free_hook (s, p, size);Then you don't need this change.
Quoted 6 lines
Quoted 3 lines
Otherwise
memcg_slab_free_hook()can be called twice for the same object. It's ok from accounting correctness perspective, because the first call will zero out the objcg pointer, but still much better to be avoided.Yes, for that one object there will be one additional uncharge call, but as you note it is handled by the !objcg check.
Got it. Thanks for the explanation!
Thanks!