linux-kernel - Re: [PATCH 01/10] mm: Assign id to every memcg-aware shrinker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180328110251.5wl3kwjhcuizyz6n@esperanza>
Date:   Wed, 28 Mar 2018 14:02:51 +0300
From:   Vladimir Davydov <vdavydov.dev@...il.com>
To:     Kirill Tkhai <ktkhai@...tuozzo.com>
Cc:     viro@...iv.linux.org.uk, hannes@...xchg.org, mhocko@...nel.org,
        akpm@...ux-foundation.org, tglx@...utronix.de,
        pombredanne@...b.com, stummala@...eaurora.org,
        gregkh@...uxfoundation.org, sfr@...b.auug.org.au, guro@...com,
        mka@...omium.org, penguin-kernel@...ove.SAKURA.ne.jp,
        chris@...is-wilson.co.uk, longman@...hat.com, minchan@...nel.org,
        hillf.zj@...baba-inc.com, ying.huang@...el.com,
        mgorman@...hsingularity.net, shakeelb@...gle.com, jbacik@...com,
        linux@...ck-us.net, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, willy@...radead.org
Subject: Re: [PATCH 01/10] mm: Assign id to every memcg-aware shrinker

On Wed, Mar 28, 2018 at 01:30:20PM +0300, Kirill Tkhai wrote:
> On 27.03.2018 18:48, Vladimir Davydov wrote:
> > On Tue, Mar 27, 2018 at 06:09:20PM +0300, Kirill Tkhai wrote:
> >>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
> >>>>>> index 8fcd9f8d7390..91b5120b924f 100644
> >>>>>> --- a/mm/vmscan.c
> >>>>>> +++ b/mm/vmscan.c
> >>>>>> @@ -159,6 +159,56 @@ unsigned long vm_total_pages;
> >>>>>>  static LIST_HEAD(shrinker_list);
> >>>>>>  static DECLARE_RWSEM(shrinker_rwsem);
> >>>>>>  
> >>>>>> +#if defined(CONFIG_MEMCG) && !defined(CONFIG_SLOB)
> >>>>>> +static DEFINE_IDA(bitmap_id_ida);
> >>>>>> +static DECLARE_RWSEM(bitmap_rwsem);
> >>>>>
> >>>>> Can't we reuse shrinker_rwsem for protecting the ida?
> >>>>
> >>>> I think it won't be better, since we allocate memory under this semaphore.
> >>>> After we use shrinker_rwsem, we'll have to allocate the memory with GFP_ATOMIC,
> >>>> which does not seems good. Currently, the patchset makes shrinker_rwsem be taken
> >>>> for a small time, just to assign already allocated memory to maps.
> >>>
> >>> AFAIR it's OK to sleep under an rwsem so GFP_ATOMIC wouldn't be
> >>> necessary. Anyway, we only need to allocate memory when we extend
> >>> shrinker bitmaps, which is rare. In fact, there can only be a limited
> >>> number of such calls, as we never shrink these bitmaps (which is fine
> >>> by me).
> >>
> >> We take bitmap_rwsem for writing to expand shrinkers maps. If we replace
> >> it with shrinker_rwsem and the memory allocation get into reclaim, there
> >> will be deadlock.
> > 
> > Hmm, AFAICS we use down_read_trylock() in shrink_slab() so no deadlock
> > would be possible. We wouldn't be able to reclaim slabs though, that's
> > true, but I don't think it would be a problem for small allocations.
> > 
> > That's how I see this. We use shrinker_rwsem to protect IDR mapping
> > shrink_id => shrinker (I still insist on IDR). It may allocate, but the
> > allocation size is going to be fairly small so it's OK that we don't
> > call shrinkers there. After we allocated a shrinker ID, we release
> > shrinker_rwsem and call mem_cgroup_grow_shrinker_map (or whatever it
> > will be called), which checks if per-memcg shrinker bitmaps need growing
> > and if they do it takes its own mutex used exclusively for protecting
> > the bitmaps and reallocates the bitmaps (we will need the mutex anyway
> > to synchronize css_online vs shrinker bitmap reallocation as the
> > shrinker_rwsem is private to vmscan.c and we don't want to export it
> > to memcontrol.c).
> 
> But what the profit of prohibiting reclaim during shrinker id allocation?
> In case of this is a IDR, it still may require 1 page, and still may get
> in after fast reclaim. If we prohibit reclaim, we'll fail to register
> the shrinker.
> 
> It's not a rare situation, when all the memory is occupied by page cache.

shrinker_rwsem doesn't block page cache reclaim, only dcache reclaim.
I don't think that dcache can occupy all available memory.

> So, we will fail to mount something in some situation.
> 
> What the advantages do we have to be more significant, than this disadvantage?

The main advantage is code simplicity.