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

[Hao Jia <jiahao.kernel@gmail.com>]

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