Messages in this thread | | | Date | Tue, 19 Apr 2022 18:57:16 +0200 | Subject | Re: [PATCH] mm: do not call add_nr_deferred() with zero deferred | From | David Hildenbrand <> |
| |
On 19.04.22 18:42, Roman Gushchin wrote: > On Tue, Apr 19, 2022 at 02:56:06PM +0200, David Hildenbrand wrote: >> On 16.04.22 02:41, Roman Gushchin wrote: >>> add_nr_deferred() is often called with next_deferred equal to 0. >>> For instance, it's happening under low memory pressure for any >>> shrinkers with a low number of cached objects. A corresponding trace >>> looks like: >>> <...>-619914 [005] .... 467456.345160: mm_shrink_slab_end: \ >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 \ >>> unused scan count 0 new scan count 0 total_scan 0 \ >>> last shrinker return val 0 >>> >>> <...>-619914 [005] .... 467456.345371: mm_shrink_slab_end: \ >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 \ >>> unused scan count 0 new scan count 0 total_scan 0 \ >>> last shrinker return val 0 >>> >>> <...>-619914 [005] .... 467456.345380: mm_shrink_slab_end: \ >>> super_cache_scan+0x0/0x1a0 0000000087027f06: nid: 1 unused \ >>> scan count 0 new scan count 0 total_scan 0 \ >>> last shrinker return val 0 >>> >>> This lead to unnecessary checks and atomic operations, which can be >>> avoided by checking next_deferred for not being zero before calling >>> add_nr_deferred(). In this case the mm_shrink_slab_end trace point >>> will get a potentially slightly outdated "new scan count" value, but >>> it's totally fine. >> >> Sufficient improvement to justify added complexity for anybody reading >> that code? > > I don't have any numbers and really doubt the difference is significant, > however the added complexity is also small: one "if" statement. > Anyway, if you feel strongly against this change, I'm fine with dropping it. >
No strong opinion, naturally, more conditions make the code harder to read -- that's why I'm asking.
>> >>> >>> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> >>> --- >>> mm/vmscan.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/vmscan.c b/mm/vmscan.c >>> index d4a7d2bd276d..19d3d4fa1aad 100644 >>> --- a/mm/vmscan.c >>> +++ b/mm/vmscan.c >>> @@ -808,7 +808,10 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl, >>> * move the unused scan count back into the shrinker in a >>> * manner that handles concurrent updates. >>> */ >>> - new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl); >>> + if (next_deferred) >>> + new_nr = add_nr_deferred(next_deferred, shrinker, shrinkctl); >>> + else >>> + new_nr = nr; >>> >>> trace_mm_shrink_slab_end(shrinker, shrinkctl->nid, freed, nr, new_nr, total_scan); >>> return freed; >> >> And if we still want to do this optimization, why not put it into >> add_nr_deferred()? > > Because of the semantics of add_nr_deferred(), which returns the deferred value. > It's not used for anything except tracing, so maybe it's a place for another > change.
Skimming over the code I somehow missed that add_nr_deferred() doesn't have "nr" naturally available.
LGTM
Acked-by: David Hildenbrand <david@redhat.com>
-- Thanks,
David / dhildenb
| |