On Fri, 03 Aug 2018 18:36:14 +0300 Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> 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?
