Received-SPF: pass (google.com: domain of linux-kernel+bounces-210143-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240608155316.451600-1-flintglass@gmail.com> <20240608155316.451600-3-flintglass@gmail.com>
 <CAJD7tkZAkzUfbXY3C0QOGqCyjQZeiuGzkZac4hmogOoh=yoZsw@mail.gmail.com>
In-Reply-To: <CAJD7tkZAkzUfbXY3C0QOGqCyjQZeiuGzkZac4hmogOoh=yoZsw@mail.gmail.com>
From: Takero Funaki <flintglass@gmail.com>
Date: Wed, 12 Jun 2024 00:21:13 +0900
Message-ID: <CAPpoddf0ysCG=s5ixbOZkXjmcB0t_eqLOs9xhdqZHiWnYY4_Wg@mail.gmail.com>
Subject: Re: [PATCH v1 2/3] mm: zswap: fix global shrinker error handling logic
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Jonathan Corbet <corbet@lwn.net>, 
	Andrew Morton <akpm@linux-foundation.org>, 
	Domenico Cerasuolo <cerasuolodomenico@gmail.com>, linux-mm@kvack.org, linux-doc@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Thanks a lot for your comments.


On 2024/06/11 5:27, Yosry Ahmed wrote:
>>                 unsigned long nr_to_walk = 1;
>>
>> +               if (!list_lru_count_one(&zswap_list_lru, nid, memcg))
>> +                       continue;
>> +               ++stored;
>>                 shrunk += list_lru_walk_one(&zswap_list_lru, nid, memcg,
>>                                             &shrink_memcg_cb, NULL, &nr_to_walk);
>>         }
>> +
>> +       if (!stored)
>> +               return -ENOENT;
>> +
>
> Can't we just check nr_to_walk here and return -ENOENT if it remains as 1?
>
> Something like:
>
> if (nr_to_walk)
>     return -ENOENT;
> if (!shrunk)
>     return -EAGAIN;
> return 0;
>

ah, the counting step can be removed. I will change it in v2.

>>         return shrunk ? 0 : -EAGAIN;
>>  }
>>
>> @@ -1418,12 +1425,18 @@ static void shrink_worker(struct work_struct *w)
>>  {
>>         struct mem_cgroup *memcg = NULL;
>>         struct mem_cgroup *next_memcg;
>> -       int ret, failures = 0;
>> +       int ret, failures = 0, progress;
>>         unsigned long thr;
>>
>>         /* Reclaim down to the accept threshold */
>>         thr = zswap_accept_thr_pages();
>>
>> +       /*
>> +        * We might start from the last memcg.
>> +        * That is not a failure.
>> +        */
>> +       progress = 1;
>> +
>>         /* global reclaim will select cgroup in a round-robin fashion.
>>          *
>>          * We save iteration cursor memcg into zswap_next_shrink,
>> @@ -1461,9 +1474,12 @@ static void shrink_worker(struct work_struct *w)
>>                  */
>>                 if (!memcg) {
>>                         spin_unlock(&zswap_shrink_lock);
>> -                       if (++failures == MAX_RECLAIM_RETRIES)
>> +
>> +                       /* tree walk completed but no progress */
>> +                       if (!progress && ++failures == MAX_RECLAIM_RETRIES)
>>                                 break;
>
> It seems like we may keep iterating the entire hierarchy a lot of
> times as long as we are making any type of progress. This doesn't seem
> right.
>

Since shrink_worker evicts only one page per tree walk when there is
only one memcg using zswap, I believe this is the intended behavior.
Even if we choose to break the loop more aggressively, it would only
be postponing the problem because pool_limit_hit will trigger the
worker again.

I agree the existing approach is inefficient. It might be better to
change the 1 page in a round-robin strategy.

>>
>> +                       progress = 0;
>>                         goto resched;
>>                 }
>>
>> @@ -1493,10 +1509,15 @@ static void shrink_worker(struct work_struct *w)
>>                 /* drop the extra reference */
>>                 mem_cgroup_put(memcg);
>>
>> -               if (ret == -EINVAL)
>> -                       break;
>> +               /* not a writeback candidate memcg */
>> +               if (ret == -EINVAL || ret == -ENOENT)
>> +                       continue;
>> +
>
> We should probably return -ENOENT for memcg with writeback disabled as well.
>
>>                 if (ret && ++failures == MAX_RECLAIM_RETRIES)
>>                         break;
>> +
>> +               ++progress;
>> +               /* reschedule as we performed some IO */
>>  resched:
>>                 cond_resched();
>>         } while (zswap_total_pages() > thr);
>> --
>> 2.43.0
>>