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

[Hao Jia]

On 2026/6/23 07:36, Yosry Ahmed wrote:
> > +/*
> > + * Walk the memcg tree and write back zswap pages until the
> > + * (lower_pages, upper_pages) window closes, or abort encounter
> > + * MAX_RECLAIM_RETRIES times of the following conditions:
> > + * - No writeback-candidate memcgs found in a memcg tree walk.
> > + * - Shrinking a writeback-candidate memcg failed.
> > + *
> > + * For shrink_worker(), it passes lower=thr and upper=zswap_total_pages().
> > + * The @upper limit is refreshed in each iteration by re-evaluating
> > + * zswap_total_pages(), and the window closes once the total falls
> > + * below the threshold.
>
> This is the wrong abstraction level, and it's obvious by the fact that
> the function calls zswap_total_pages() again to recalcualte
> 'upper_pages'. It gets much worse in the next patch as well.
>
> The lower_pages and upper_pages thing is also unnecessarily hard to
> follow.
>
> The core of the reuse here is the retry logic. So maybe keep the memcg
> iteration in the callers, and define a function that takes in one memcg
> and reclaims one batch from it? failures and attempts can be passed into
> the function to maintain the state across scans of different memcgs,
> like zswap_shrink_walk_arg?
>
> WDYT?


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;

    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;
    }
}

We could also fold the logic of zswap_shrink_no_candidate() into 
zswap_shrink_one(), but adding a !memcg check inside zswap_shrink_one() 
feels a bit awkward.

WDYT?

Thanks,
Hao