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

[Hao Jia]

On 2026/6/25 01:00, Yosry Ahmed wrote:
> On Wed, Jun 24, 2026 at 4:55 AM Hao Jia <jiahao.kernel@xxxxxxxxx> wrote:
> > On 2026/6/23 07:36, Yosry Ahmed wrote:

> > Perhaps something like this?
> >
> > struct zswap_shrink_state {
> >       int attempts;
> >       int failures;
> >       bool stop;
> > };
> >
> > static bool zswap_shrink_no_candidate(struct zswap_shrink_state *s)
> > {
> >       if (!s->attempts && ++s->failures == MAX_RECLAIM_RETRIES)
> >           return true;
> >
> >       s->attempts = 0;
> >       return false;
> > }
> >
> > 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.


> >       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;
> >
> >           cond_resched();
> >
> >           memcg = zswap_iter_global();
> >           if (!memcg) {
> >               if (zswap_shrink_no_candidate(&s))
> >                   break;
> >               continue;
> >           }
> >
> >           zswap_shrink_one(memcg, &s);
> >           /* Drop the extra reference taken by the iterator. */
> >           mem_cgroup_put(memcg);
> >           if (s.stop)
> >               break;
> >       }
> > }

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

/* Track progress of a memcg-tree writeback walk. */
struct zswap_shrink_state {
    int attempts;
    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.
 *
 * 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,
                 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();
        ret = zswap_shrink_one(memcg, &s);
        /* drop the extra reference taken by zswap_iter_global() */
        mem_cgroup_put(memcg);
        if (ret == -EAGAIN)
            break;
    }
}