On 17.05.2018 07:16, Vladimir Davydov wrote: > On Tue, May 15, 2018 at 05:49:59PM +0300, Kirill Tkhai wrote: >>>> @@ -589,13 +647,7 @@ static unsigned long shrink_slab(gfp_t gfp_mask, int nid, >>>> .memcg = memcg, >>>> }; >>>> >>>> - /* >>>> - * If kernel memory accounting is disabled, we ignore >>>> - * SHRINKER_MEMCG_AWARE flag and call all shrinkers >>>> - * passing NULL for memcg. >>>> - */ >>>> - if (memcg_kmem_enabled() && >>>> - !!memcg != !!(shrinker->flags & SHRINKER_MEMCG_AWARE)) >>>> + if (!!memcg != !!(shrinker->flags & SHRINKER_MEMCG_AWARE)) >>>> continue; >>> >>> I want this check gone. It's easy to achieve, actually - just remove the >>> following lines from shrink_node() >>> >>> if (global_reclaim(sc)) >>> shrink_slab(sc->gfp_mask, pgdat->node_id, NULL, >>> sc->priority); >> >> This check is not related to the patchset. > > Yes, it is. This patch modifies shrink_slab which is used only by > shrink_node. Simplifying shrink_node along the way looks right to me. shrink_slab() is used not only in this place. I does not seem a trivial change for me. >> Let's don't mix everything in the single series of patches, because >> after your last remarks it will grow at least up to 15 patches. > > Most of which are trivial so I don't see any problem here. > >> This patchset can't be responsible for everything. > > I don't understand why you balk at simplifying the code a bit while you > are patching related functions anyway. Because this function is used in several places, and we have some particulars on root_mem_cgroup initialization, and this function called from these places with different states of root_mem_cgroup. It does not seem trivial fix for me. Let's do it on top of the series later, what is the problem? It does not seem critical problem. Kirill
