Re: [PATCH v3 2/2] mm: zswap: fix global shrinker error handling logic

[Takero Funaki]

Thanks for your comments.


2024年7月26日(金) 12:21 Chengming Zhou <chengming.zhou@xxxxxxxxx>:
> > and, the reasons to (not) increment the progress:
> >
> > @@ -1387,10 +1407,20 @@ static void shrink_worker(struct work_struct *w)
> >                  /* drop the extra reference */
> >                  mem_cgroup_put(memcg);
> >
> > -               if (ret == -EINVAL)
> > -                       break;
> > +               /*
> > +                * 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 progress
> > +                * and failures.
> > +                */
> > +               if (ret == -ENOENT)
> > +                       continue;
> > +
> >                  if (ret && ++failures == MAX_RECLAIM_RETRIES)
> >                          break;
> > +
> > +               /* completed writeback or incremented failures */
> > +               ++progress;
>
> Maybe the name "progress" is a little confusing here? "progress" sounds
> to me that we have some writeback completed.
>
> But actually it just means we have encountered some candidates, right?
>
> Thanks.
>
>

Yes, the `++progress` counts both error and success as an iteration
progress for valid memcgs (not writeback amount). Incrementing only on
success will overly increment failures counter if there is only one
memcg, one from writeback failure and one from tree walk ends, the
worker aborts on 8 failures instead of 16.
`++candidates;` would be better? replacing the name and fixing commit
messages for v4.