Re: [v7 PATCH 12/12] mm: vmscan: shrink deferred objects proportional to priority

[Yang Shi]

On Thu, Feb 11, 2021 at 10:52 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> On 2/11/21 6:29 PM, Yang Shi wrote:
> > On Thu, Feb 11, 2021 at 5:10 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
> >> >       trace_mm_shrink_slab_start(shrinker, shrinkctl, nr,
> >> >                                  freeable, delta, total_scan, priority);
> >> > @@ -737,10 +708,9 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
> >> >               cond_resched();
> >> >       }
> >> >
> >> > -     if (next_deferred >= scanned)
> >> > -             next_deferred -= scanned;
> >> > -     else
> >> > -             next_deferred = 0;
> >> > +     next_deferred = max_t(long, (nr - scanned), 0) + total_scan;
> >>
> >> And here's the bias I think. Suppose we scanned 0 due to e.g. GFP_NOFS. We count
> >> as newly deferred both the "delta" part of total_scan, which is fine, but also
> >> the "nr >> priority" part, where we failed to our share of the "reduce
> >> nr_deferred" work, but I don't think it means we should also increase
> >> nr_deferred by that amount of failed work.
> >
> > Here "nr" is the saved deferred work since the last scan, "scanned" is
> > the scanned work in this round, total_scan is the *unscanned" work
> > which is actually "total_scan - scanned" (total_scan is decreased by
> > scanned in each loop). So, the logic is "decrease any scanned work
> > from deferred then add newly unscanned work to deferred". IIUC this is
> > what "deferred" means even before this patch.
>
> Hm I thought the logic was "increase by any new work (delta) that wasn't done,
> decrease by old deferred work that was done now". My examples with scanned = 0
> and scanned = total_work (total_work before subtracting scanned from it) should
> demonstrate that the logic is different with your patch.

I think we are on the same page about the logic. But I agree the
formula implemented in the code is wrong.

>
> >> OTOH if we succeed and scan exactly the whole goal, we are subtracting from
> >> nr_deferred both the "nr >> priority" part, which is correct, but also delta,
> >> which was new work, not deferred one, so that's incorrect IMHO as well.
> >
> > I don't think so. The deferred comes from new work, why not dec new
> > work from deferred?
> >
> > And, the old code did:
> >
> > if (next_deferred >= scanned)
> >                 next_deferred -= scanned;
> >         else
> >                 next_deferred = 0;
> >
> > IIUC, it also decreases the new work (the scanned includes both last
> > deferred and new delata).
>
> Yes, but in the old code, next_deferred starts as
>
> nr = count_nr_deferred()...
> total_scan = nr;
> delta = ... // something based on freeable
> total_scan += delta;
> next_deferred = total_scan; // in the common case total_scan >= 0
>
> ... and that's "total_scan" before "scanned" is subtracted from it, so it
> includes the new_work ("delta"), so then it's OK to do "next_deferred -= scanned";
>
> I still think your formula is (unintentionally) changing the logic. You can also
> look at it from different angle, it's effectively (without the max_t() part) "nr
> - scanned + total_scan" where total_scan is actually "total_scan - scanned" as
> you point your yourself. So "scanned" is subtracted twice? That can't be correct...

Yes, I think you are right, it can not be correct. Actually I wanted
plus the unscanned delta part to the next_deferred. But my formula
actually not only decs scanned twice but also adds unscanned deferred
back again. So it seems the formula suggested by you is correct. Will
correct this in v8. Thanks a lot for helping get out of the maze. Will
add some notes right before the formula as well.

>
> >> So the calculation should probably be something like this?
> >>
> >>         next_deferred = max_t(long, nr + delta - scanned, 0);
> >>
> >> Thanks,
> >> Vlastimil
> >>
> >> > +     next_deferred = min(next_deferred, (2 * freeable));
> >> > +
> >> >       /*
> >> >        * move the unused scan count back into the shrinker in a
> >> >        * manner that handles concurrent updates.
> >> >
> >>
> >
>