| Message ID | 20210524233946.20352-3-vbabka@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SLUB: use local_lock for kmem_cache_cpu protection and reduce disabling irqs | expand |
On Tue, 25 May 2021, Vlastimil Babka wrote: > validate_slab_cache() is called either to handle a sysfs write, or from a > self-test context. In both situations it's straightforward to preallocate a > private object bitmap instead of grabbing the shared static one meant for > critical sections, so let's do that. Acked-by: Christoph Lameter <cl@linux.com>
On Tue, May 25, 2021 at 01:39:22AM +0200, Vlastimil Babka wrote: > validate_slab_cache() is called either to handle a sysfs write, or from a > self-test context. In both situations it's straightforward to preallocate a > private object bitmap instead of grabbing the shared static one meant for > critical sections, so let's do that. > > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > > <SNIP> > > @@ -4685,10 +4685,17 @@ static long validate_slab_cache(struct kmem_cache *s) > int node; > unsigned long count = 0; > struct kmem_cache_node *n; > + unsigned long *obj_map; > + > + obj_map = bitmap_alloc(oo_objects(s->oo), GFP_KERNEL); > + if (!obj_map) > + return -ENOMEM; > Most callers of validate_slab_cache don't care about the return value except when the validate sysfs file is written. Should a simply informational message be displayed for -ENOMEM in case a writer to validate fails and it's not obvious it was because of an allocation failure? It's a fairly minor concern so whether you add a message or not Acked-by: Mel Gorman <mgorman@techsingularity.net>
On 5/25/21 12:17 PM, Mel Gorman wrote: > On Tue, May 25, 2021 at 01:39:22AM +0200, Vlastimil Babka wrote: >> validate_slab_cache() is called either to handle a sysfs write, or from a >> self-test context. In both situations it's straightforward to preallocate a >> private object bitmap instead of grabbing the shared static one meant for >> critical sections, so let's do that. >> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> >> <SNIP> >> >> @@ -4685,10 +4685,17 @@ static long validate_slab_cache(struct kmem_cache *s) >> int node; >> unsigned long count = 0; >> struct kmem_cache_node *n; >> + unsigned long *obj_map; >> + >> + obj_map = bitmap_alloc(oo_objects(s->oo), GFP_KERNEL); >> + if (!obj_map) >> + return -ENOMEM; >> > > > Most callers of validate_slab_cache don't care about the return value > except when the validate sysfs file is written. Should a simply > informational message be displayed for -ENOMEM in case a writer to > validate fails and it's not obvious it was because of an allocation > failure? he other callers are all in the effectively dead resiliency_test() code, which has meanwhile been replaced in mmotm by kunit tests meanwhile. But it's true those don't check the results either for now. > It's a fairly minor concern so whether you add a message or not I think I'll rather fix up the tests. Or do you mean that -ENOMEM for a sysfs write is also not enough and there should be a dmesg explanation for that case? > Acked-by: Mel Gorman <mgorman@techsingularity.net> Thanks!
On Tue, May 25, 2021 at 12:36:52PM +0200, Vlastimil Babka wrote: > > Most callers of validate_slab_cache don't care about the return value > > except when the validate sysfs file is written. Should a simply > > informational message be displayed for -ENOMEM in case a writer to > > validate fails and it's not obvious it was because of an allocation > > failure? > > he other callers are all in the effectively dead resiliency_test() code, which > has meanwhile been replaced in mmotm by kunit tests meanwhile. But it's true > those don't check the results either for now. > Ok. > > It's a fairly minor concern so whether you add a message or not > > I think I'll rather fix up the tests. Or do you mean that -ENOMEM for a sysfs > write is also not enough and there should be a dmesg explanation for that case? > I mean the -ENOMEM for a sysfs write. While it's very unlikely, it might would explain an unexpected write failure.
On 5/25/21 1:33 PM, Mel Gorman wrote: > On Tue, May 25, 2021 at 12:36:52PM +0200, Vlastimil Babka wrote: >> > Most callers of validate_slab_cache don't care about the return value >> > except when the validate sysfs file is written. Should a simply >> > informational message be displayed for -ENOMEM in case a writer to >> > validate fails and it's not obvious it was because of an allocation >> > failure? >> >> he other callers are all in the effectively dead resiliency_test() code, which >> has meanwhile been replaced in mmotm by kunit tests meanwhile. But it's true >> those don't check the results either for now. >> > > Ok. > >> > It's a fairly minor concern so whether you add a message or not >> >> I think I'll rather fix up the tests. Or do you mean that -ENOMEM for a sysfs >> write is also not enough and there should be a dmesg explanation for that case? >> > > I mean the -ENOMEM for a sysfs write. While it's very unlikely, it might > would explain an unexpected write failure. On second thought, a failed GFP_KERNEL allocation will already generate a prominent warning, so an extra message looks arbitrary.
diff --git a/mm/slub.c b/mm/slub.c index 4c876749f322..ed0f2620b19b 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -4622,7 +4622,8 @@ static int count_total(struct page *page) #endif #ifdef CONFIG_SLUB_DEBUG -static void validate_slab(struct kmem_cache *s, struct page *page) +static void validate_slab(struct kmem_cache *s, struct page *page, + unsigned long *obj_map) { void *p; void *addr = page_address(page); @@ -4634,7 +4635,7 @@ static void validate_slab(struct kmem_cache *s, struct page *page) goto unlock; /* Now we know that a valid freelist exists */ - map = get_map(s, page); + __fill_map(obj_map, s, page); for_each_object(p, s, addr, page->objects) { u8 val = test_bit(__obj_to_index(s, addr, p), map) ? SLUB_RED_INACTIVE : SLUB_RED_ACTIVE; @@ -4642,13 +4643,12 @@ static void validate_slab(struct kmem_cache *s, struct page *page) if (!check_object(s, page, p, val)) break; } - put_map(map); unlock: slab_unlock(page); } static int validate_slab_node(struct kmem_cache *s, - struct kmem_cache_node *n) + struct kmem_cache_node *n, unsigned long *obj_map) { unsigned long count = 0; struct page *page; @@ -4657,7 +4657,7 @@ static int validate_slab_node(struct kmem_cache *s, spin_lock_irqsave(&n->list_lock, flags); list_for_each_entry(page, &n->partial, slab_list) { - validate_slab(s, page); + validate_slab(s, page, obj_map); count++; } if (count != n->nr_partial) @@ -4668,7 +4668,7 @@ static int validate_slab_node(struct kmem_cache *s, goto out; list_for_each_entry(page, &n->full, slab_list) { - validate_slab(s, page); + validate_slab(s, page, obj_map); count++; } if (count != atomic_long_read(&n->nr_slabs)) @@ -4685,10 +4685,17 @@ static long validate_slab_cache(struct kmem_cache *s) int node; unsigned long count = 0; struct kmem_cache_node *n; + unsigned long *obj_map; + + obj_map = bitmap_alloc(oo_objects(s->oo), GFP_KERNEL); + if (!obj_map) + return -ENOMEM; flush_all(s); for_each_kmem_cache_node(s, node, n) - count += validate_slab_node(s, n); + count += validate_slab_node(s, n, obj_map); + + bitmap_free(obj_map); return count; }
validate_slab_cache() is called either to handle a sysfs write, or from a self-test context. In both situations it's straightforward to preallocate a private object bitmap instead of grabbing the shared static one meant for critical sections, so let's do that. Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)