On Thu, Jul 05, 2018 at 03:10:30PM -0700, Andrew Morton wrote: > On Wed, 4 Jul 2018 18:51:12 +0300 Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote: > > > > - why aren't we decreasing shrinker_nr_max in > > > unregister_memcg_shrinker()? That's easy to do, avoids pointless > > > work in shrink_slab_memcg() and avoids memory waste in future > > > prealloc_memcg_shrinker() calls. > > > > You sure, but there are some things. Initially I went in the same way > > as memcg_nr_cache_ids is made and just took the same x2 arithmetic. > > It never decreases, so it looked good to make shrinker maps like it. > > It's the only reason, so, it should not be a problem to rework. > > > > The only moment is Vladimir strongly recommends modularity, i.e. > > to have memcg_shrinker_map_size and shrinker_nr_max as different variables. > > For what reasons? Having the only global variable updated in vmscan.c and used in memcontrol.c or vice versa didn't look good to me. So I suggested to introduce two separate static variables: one for max shrinker id (local to vmscan.c) and another for max allocated size of per memcg shrinker maps (local to memcontrol.c). Having the two variables instead of one allows us to define a clear API between memcontrol.c and shrinker infrastructure without sharing variables, which makes the code easier to follow IMHO. It is also more flexible: with the two variables we can decrease shrinker_id_max when a shrinker is destroyed so that we can speed up shrink_slab - this is fairly easy to do - but leave per memcg shrinker maps the same size, because shrinking them is rather complicated and doesn't seem to be worthwhile - the workload is likely to use the same amount of memory again in the future. > > > After the rework we won't be able to have this anymore, since memcontrol.c > > will have to know actual shrinker_nr_max value and it will have to be exported. Not necessarily. You can pass max shrinker id instead of the new id to memcontrol.c in function arguments. But as I said before, I really don't think that shrinking per memcg maps would make much sense. > > > > Could this be a problem?
