Re: [PATCH v4 2/5] mm/zswap: Factor writeback loop out of shrink_worker()

[Yosry Ahmed]

> >> static long zswap_shrink_one(struct mem_cgroup *memcg,
> >>                    struct zswap_shrink_state *s)
> >> {
> >>       long shrunk;
> >>
> >>       shrunk = shrink_memcg(memcg, NR_ZSWAP_WB_BATCH);
> >>       if (shrunk == -ENOENT)
> >>           return 0;
> >>
> >>       s->attempts++;
> >>       if (shrunk <= 0 && ++s->failures == MAX_RECLAIM_RETRIES)
> >>           s->stop = true;
> >
> > Do we need 'stop' or can we just return a value here to indicate that
> > we should stop (e.g. -EBUSY)?
> >
>
> Perhaps we could return -EAGAIN instead of -EBUSY? This would align with
> the semantics of the memory.reclaim interface, which returns -EAGAIN
> when it reclaims fewer bytes than requested.

Hmm but -EAGAIN tells the caller to try again, while here -EAGAIN
tells the caller *not* to try again because we exhausted all retries?

> >
> > I think splitting the shrink/retry logic over 2 functions makes it
> > more difficult to follow, so yeah I think fold
> > zswap_shrink_no_candidate() into zswap_shrink_one(). Then the callers
> > only need to iterate memcgs (depending on the context) and call
> > zswap_shrink_one() for each of them.
>
> So, something like this?

Yeah, something like this :)

> /* Track progress of a memcg-tree writeback walk. */
> struct zswap_shrink_state {
>      int attempts;

While at it, I think "attempts" is really the number of scans, right?
Should we rename it? Maybe "scans" or similar?

>      int failures;
> };
>
> /*
>   * Take one step of a memcg-tree writeback walk driven by the caller's
>   * iterator, and fold the result into @s, the retry bookkeeping shared
>   * across steps. @memcg is the iterator's current memcg, or NULL once
>   * it has wrapped around after a full pass over the tree.
>   *
>   * The function returns -EAGAIN to signal the caller to abort the walk
>   * after encountering the following conditions MAX_RECLAIM_RETRIES times:
>   * - No writeback-candidate memcgs were found in a memcg tree walk.
>   * - Shrinking a writeback-candidate memcg failed.

Orthogonal to this patch, but I wonder if this can be simplified. I
wonder if these two conditions can be replaced with "shrinking a memcg
that has zswap entries failed". The "no writeback-candidate memcgs in
the tree" case seems like we should abort right away instead of
retrying?

Nhat, WDYT?

>   *
>   * Return: The number of compressed bytes written back (>= 0), or -EAGAIN
>   * once the retry budget is exhausted and the caller should abort the walk.
>   */
> static long zswap_shrink_one(struct mem_cgroup *memcg,

Nit: zswap_shrink_one_memcg()

BTW, the existing writeback logic has been broken for a while now when
memcg is disabled. I think we constantly hit the !memcg case and run
out of retries. Not sure if your patch changes this in any way, or if
you want to fix that while you're at it :)

>                   struct zswap_shrink_state *s)
> {
>      long shrunk;
>
>      /*
>       * If the iterator has completed a full pass, update the shrink state
>       * and check whether we should keep going.
>       */
>      if (!memcg) {
>          /*
>           * Continue shrinking without incrementing failures if we found
>           * candidate memcgs in the last tree walk.
>           */
>          if (!s->attempts && ++s->failures == MAX_RECLAIM_RETRIES)
>              return -EAGAIN;
>          s->attempts = 0;
>          return 0;
>      }
>
>      shrunk = shrink_memcg(memcg, NR_ZSWAP_WB_BATCH);
>
>      /*
>       * There are no writeback-candidate pages in the memcg. This is not an
>       * issue as long as we can find another memcg with pages in zswap. Skip
>       * this without incrementing attempts and failures.
>       */
>      if (shrunk == -ENOENT)
>          return 0;
>      s->attempts++;
>
>      if (shrunk <= 0 && ++s->failures == MAX_RECLAIM_RETRIES)
>          return -EAGAIN;
>
>      return shrunk;
> }
>
> static void shrink_worker(struct work_struct *w)
> {
>      struct zswap_shrink_state s = {};
>      unsigned long thr;
>
>      /* Reclaim down to the accept threshold */
>      thr = zswap_accept_thr_pages();
>
>      while (zswap_total_pages() > thr) {
>          struct mem_cgroup *memcg;
>          long ret;
>
>          cond_resched();
>
>          memcg = zswap_iter_global();

Do we still need this helper? Or should we just keep the memcg
iteration open-coded?

>          ret = zswap_shrink_one(memcg, &s);
>          /* drop the extra reference taken by zswap_iter_global() */
>          mem_cgroup_put(memcg);
>          if (ret == -EAGAIN)
>              break;
>      }
> }