lkml.org 
[lkml]   [2019]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 09/28] mm: directed shrinker work deferral
    On Mon, Nov 18, 2019 at 11:49:56AM +1100, Dave Chinner wrote:
    > On Fri, Nov 15, 2019 at 12:21:40PM -0500, Brian Foster wrote:
    > > On Fri, Nov 15, 2019 at 07:49:26AM +1100, Dave Chinner wrote:
    > > > On Mon, Nov 04, 2019 at 10:25:25AM -0500, Brian Foster wrote:
    > > > > On Fri, Nov 01, 2019 at 10:45:59AM +1100, Dave Chinner wrote:
    > > > > > From: Dave Chinner <dchinner@redhat.com>
    > > > > >
    > > > > > Introduce a mechanism for ->count_objects() to indicate to the
    > > > > > shrinker infrastructure that the reclaim context will not allow
    > > > > > scanning work to be done and so the work it decides is necessary
    > > > > > needs to be deferred.
    > > > > >
    > > > > > This simplifies the code by separating out the accounting of
    > > > > > deferred work from the actual doing of the work, and allows better
    > > > > > decisions to be made by the shrinekr control logic on what action it
    > > > > > can take.
    > > > > >
    > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
    > > > > > ---
    > > > >
    > > > > My understanding from the previous discussion(s) is that this is not
    > > > > tied directly to the gfp mask because that is not the only intended use.
    > > > > While it is currently a boolean tied to the the entire shrinker call,
    > > > > the longer term objective is per-object granularity.
    > > >
    > > > Longer term, yes, but right now such things are not possible as the
    > > > shrinker needs more context to be able to make sane per-object
    > > > decisions. shrinker policy decisions that affect the entire run
    > > > scope should be handled by the ->count operation - it's the one that
    > > > says whether the scan loop should run or not, and right now GFP_NOFS
    > > > for all filesystem shrinkers is a pure boolean policy
    > > > implementation.
    > > >
    > > > The next future step is to provide a superblock context with
    > > > GFP_NOFS to indicate which filesystem we cannot recurse into. That
    > > > is also a shrinker instance wide check, so again it's something that
    > > > ->count should be deciding.
    > > >
    > > > i.e. ->count determines what is to be done, ->scan iterates the work
    > > > that has to be done until we are done.
    > > >
    > >
    > > Sure, makes sense in general.
    > >
    > > > > I find the argument reasonable enough, but if the above is true, why do
    > > > > we move these checks from ->scan_objects() to ->count_objects() (in the
    > > > > next patch) when per-object decisions will ultimately need to be made by
    > > > > the former?
    > > >
    > > > Because run/no-run policy belongs in one place, and things like
    > > > GFP_NOFS do no change across calls to the ->scan loop. i.e. after
    > > > the first ->scan call in a loop that calls it hundreds to thousands
    > > > of times, the GFP_NOFS run/no-run check is completely redundant.
    > > >
    > >
    > > What loop is currently called hundreds to thousands of times that this
    > > change prevents? AFAICT the current nofs checks in the ->scan calls
    > > explicitly terminate the scan loop.
    >
    > Right, but when we are in GFP_KERNEL context, every call to ->scan()
    > checks it and says "ok". If we are scanning tens of thousands of
    > objects in a scan, and we are using a befault batch size of 128
    > objects per scan, then we have hundreds of calls in a single scan
    > loop that check the GFP context and say "ok"....
    >
    > > So we're effectively saving a
    > > function call by doing this earlier in the count ->call. (Nothing wrong
    > > with that, I'm just not following the numbers used in this reasoning..).
    >
    > It's the don't terminate case. :)
    >

    Oh, I see. You're talking about the number of executions of the gfp
    check itself. That makes sense, though my understanding is that we'll
    ultimately have a similar check anyways if we want per-object
    granularity based on the allocation constraints of the current context.
    OTOH, the check would still occur only once with an alloc flags field in
    the shrinker structure too, FWIW.

    > > > Once we introduce a new policy that allows the fs shrinker to do
    > > > careful reclaim in GFP_NOFS conditions, we need to do substantial
    > > > rework the shrinker scan loop and how it accounts the work that is
    > > > done - we now have at least 3 or 4 different return counters
    > > > (skipped because locked, skipped because referenced,
    > > > reclaimed, deferred reclaim because couldn't lock/recursion) and
    > > > the accounting and decisions to be made are a lot more complex.
    > > >
    > >
    > > Yeah, that's generally what I expected from your previous description.
    > >
    > > > In that case, the ->count function will drop the GFP_NOFS check, but
    > > > still do all the other things is needs to do. The GFP_NOFS check
    > > > will go deep in the guts of the shrinker scan implementation where
    > > > the per-object recursion problem exists. But for most shrinkers,
    > > > it's still going to be a global boolean check...
    > > >
    > >
    > > So once the nofs checks are lifted out of the ->count callback and into
    > > the core shrinker, is there still a use case to defer an entire ->count
    > > instance from the callback?
    >
    > Not right now. There may be in future, but I don't want to make
    > things more complex than they need to be by trying to support
    > functionality that isn't used.
    >

    Ok, but do note that the reason I ask is to touch on simply whether it's
    worth putting this in the ->scan callback at all. It's not like _not_
    doing that is some big complexity adjustment. ;)

    > > > If people want to call avoiding repeated, unnecessary evaluation of
    > > > the same condition hundreds of times instead of once "unnecessary
    > > > churn", then I'll drop it.
    > > >
    > >
    > > I'm not referring to the functional change as churn. What I was
    > > referring to is that we're shuffling around the boilerplate gfp checking
    > > code between the different shrinker callbacks, knowing that it's
    > > eventually going to be lifted out, when we could potentially just lift
    > > that code up a level now.
    >
    > I don't think that lifting it up will save much code at all, once we
    > add all the gfp mask intialisation to all the shrinkers, etc. It's
    > just means we can't look at the shrinker implementation and know
    > that it can't run in GFP_NOFS context - we have to go look up
    > where it is instantiated instead to see if there are gfp context
    > constraints.
    >
    > I think it's better where it is, documenting the constraints the
    > shrinker implementation runs under in the implementation itself...
    >

    Fair enough.. I don't necessarily agree that this is the best approach,
    but the implementation is reasonable enough that I certainly don't
    object to it (provided the fragility nits are addressed) and I don't
    feel particularly tied to the suggested alternative. At the end of the
    day this isn't a lot of code and it's not difficult to change (which it
    probably will). I just wanted to make sure the alternative was fairly
    considered and to test the reasoning for the approach a bit. I'll
    move along from this topic on review of the next version...

    Brian

    > Cheers,
    >
    > Dave.
    > --
    > Dave Chinner
    > david@fromorbit.com
    >

    \
     
     \ /
      Last update: 2019-11-19 16:12    [W:2.863 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site