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

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

On 2026/6/26 01:59, Yosry Ahmed wrote:
>>>> 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?
> 

Okay, let's go with -EBUSY.

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

Perhaps something like the following is what you had in mind? I've 
drafted the implementation below to make it easier for Nhat to compare 
with the previous behavior.


>>    *
>>    * 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 :)

Yes, I'd be happy to do that. However, would it be better to submit a 
separate fix patch or combine it with this one?

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

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


/* Track progress of a memcg-tree writeback walk. */
struct zswap_shrink_state {
     int scans;
     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 -EBUSY to signal the caller to abort the walk when
  * either of the following occurs:
  * - A full pass over the tree found no writeback-candidate memcg.
  * - Shrinking a writeback-candidate memcg failed MAX_RECLAIM_RETRIES 
times.
  *
  * When memory cgroup is disabled, the iterator always yields NULL. All
  * zswap entries then live on the root list_lru, so NULL is treated as the
  * root memcg and shrunk directly rather than as a completed tree pass.
  *
  * Return: The number of compressed bytes written back (>= 0), or -EBUSY
  * when the caller should abort the walk.
  */
static long zswap_shrink_one_memcg(struct mem_cgroup *memcg,
                    struct zswap_shrink_state *s)
{
     bool disabled = mem_cgroup_disabled();
     long shrunk;

     /*
      * If the iterator has completed a full pass, update the shrink state
      * and check whether we should keep going.
      * With memcg disabled the iterator always yields NULL, so fall through
      * and shrink the root memcg directly instead.
      */
     if (!memcg && !disabled) {
         /*
          * Abort if no writeback-candidate memcgs in the last tree walk.
          * Otherwise reset the scans count and continue.
          */
         if (!s->scans)
             return -EBUSY;
         s->scans = 0;
         return 0;
     }

     shrunk = shrink_memcg(memcg, NR_ZSWAP_WB_BATCH);

     /*
      * There are no writeback-candidate pages in the memcg. With memcg
      * enabled this is not an issue as long as we can find another memcg
      * with pages in zswap, so skip without counting it as a candidate.
      * With memcg disabled the root LRU is the only target, so we should
      * abort if it has no writeback-candidate pages.
      */
     if (shrunk == -ENOENT)
         return disabled ? -EBUSY : 0;
     s->scans++;

     if (shrunk <= 0 && ++s->failures == MAX_RECLAIM_RETRIES)
         return -EBUSY;

     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();

     /*
      * Global reclaim will select cgroup in a round-robin fashion from all
      * online memcgs, but memcgs that have no pages in zswap and
      * writeback-disabled memcgs (memory.zswap.writeback=0) are not
      * candidates for shrinking.
      *
      * We save iteration cursor memcg into zswap_next_shrink,
      * which can be modified by the offline memcg cleaner
      * zswap_memcg_offline_cleanup().
      *
      * Since the offline cleaner is called only once, we cannot leave an
      * offline memcg reference in zswap_next_shrink.
      * We can rely on the cleaner only if we get online memcg under lock.
      *
      * If we get an offline memcg, we cannot determine if the cleaner has
      * already been called or will be called later. We must put back the
      * reference before returning from this function. Otherwise, the
      * offline memcg left in zswap_next_shrink will hold the reference
      * until the next run of shrink_worker().
      */
     while (zswap_total_pages() > thr) {
         struct mem_cgroup *memcg;
         long ret;

         cond_resched();
         /*
          * Start shrinking from the next memcg after zswap_next_shrink.
          * When the offline cleaner has already advanced the cursor,
          * advancing the cursor here overlooks one memcg, but this
          * should be negligibly rare.
          *
          * If we get an online memcg, keep the extra reference in case
          * the original one obtained by mem_cgroup_iter() is dropped by
          * zswap_memcg_offline_cleanup() while we are shrinking the
          * memcg.
          */
         spin_lock(&zswap_shrink_lock);
         do {
             memcg = mem_cgroup_iter(NULL, zswap_next_shrink, NULL);
             zswap_next_shrink = memcg;
         } while (memcg && !mem_cgroup_tryget_online(memcg));
         spin_unlock(&zswap_shrink_lock);

         ret = zswap_shrink_one_memcg(memcg, &s);
         /* drop the extra reference taken above */
         mem_cgroup_put(memcg);
         if (ret == -EBUSY)
             break;
     }
}


Thanks,
Hao