lkml.org 
[lkml]   [2018]   [Aug]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mm: Use special value SHRINKER_REGISTERING instead list_empty() check
On Fri, 03 Aug 2018 18:36:14 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:

> The patch introduces a special value SHRINKER_REGISTERING to use instead
> of list_empty() to detect a semi-registered shrinker.
>
> This should be clearer for a reader since "list is empty" is not
> an intuitive state of a shrinker), and this gives a better assembler
> code:
>
> Before:
> callq <idr_find>
> mov %rax,%r15
> test %rax,%rax
> je <shrink_slab_memcg+0x1d5>
> mov 0x20(%rax),%rax
> lea 0x20(%r15),%rdx
> cmp %rax,%rdx
> je <shrink_slab_memcg+0xbd>
> mov 0x8(%rsp),%edx
> mov %r15,%rsi
> lea 0x10(%rsp),%rdi
> callq <do_shrink_slab>
>
> After:
> callq <idr_find>
> mov %rax,%r15
> lea -0x1(%rax),%rax
> cmp $0xfffffffffffffffd,%rax
> ja <shrink_slab_memcg+0x1cd>
> mov 0x8(%rsp),%edx
> mov %r15,%rsi
> lea 0x10(%rsp),%rdi
> callq ffffffff810cefd0 <do_shrink_slab>
>
> Also, improve the comment.

All this isn't terribly nice. Why can't we avoid installing the
shrinker into the idr until it is fully initialized?

Or extend the down_write(shrinker_rwsem) coverage so it protects the
entire initialization, instead of only in the prealloc_memcg_shrinker()
part of that initialization. This is not as good - it would be better
to do all the initialization locklessly then just install the fully
initialized thing under the lock.

> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -170,6 +170,21 @@ static LIST_HEAD(shrinker_list);
> static DECLARE_RWSEM(shrinker_rwsem);
>
> #ifdef CONFIG_MEMCG_KMEM
> +
> +/*
> + * There is a window between prealloc_shrinker()
> + * and register_shrinker_prepared(). We don't want
> + * to clear bit of a shrinker in such the state
> + * in shrink_slab_memcg(), since this will impose
> + * restrictions on a code registering a shrinker
> + * (they would have to guarantee, their LRU lists
> + * are empty till shrinker is completely registered).
> + * So, we use this value to detect the situation,
> + * when id is assigned, but shrinker is not completely
> + * registered yet.
> + */

This comment is still quite hard to understand. Could you please spend
a little more time over it?


\
 
 \ /
  Last update: 2018-08-04 00:51    [W:0.042 / U:0.908 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site