On 2/11/21 6:29 PM, Yang Shi wrote:

On Thu, Feb 11, 2021 at 5:10 AM Vlastimil Babka [off-list ref] wrote:

quoted

quoted

      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.

quoted

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...

quoted

So the calculation should probably be something like this?

        next_deferred = max_t(long, nr + delta - scanned, 0);

Thanks,
Vlastimil

quoted

+     next_deferred = min(next_deferred, (2 * freeable));
+
      /*
       * move the unused scan count back into the shrinker in a
       * manner that handles concurrent updates.