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

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

On 2026/6/25 01:00, Yosry Ahmed wrote:
> On Wed, Jun 24, 2026 at 4:55 AM Hao Jia <jiahao.kernel@gmail.com> 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;
     }
}