On 2024/3/26 07:50, Yosry Ahmed wrote:
> The cleanup code in zswap_store() is not pretty, particularly the
> 'shrink' label at the bottom that ends up jumping between cleanup
> labels.
>
> Instead of having a dedicated label to shrink the pool, just use
> zswap_pool_reached_full directly to figure out if the pool needs
> shrinking. zswap_pool_reached_full should be true if and only if the
> pool needs shrinking.
>
> The only caveat is that the value of zswap_pool_reached_full may be
> changed by concurrent zswap_store() calls between checking the limit and
> testing zswap_pool_reached_full in the cleanup code. This is fine
> because:
> - If zswap_pool_reached_full was true during limit checking then became
> false during the cleanup code, then someone else already took care of
> shrinking the pool and there is no need to queue the worker. That
> would be a good change.
> - If zswap_pool_reached_full was false during limit checking then became
> true during the cleanup code, then someone else hit the limit
> meanwhile. In this case, both threads will try to queue the worker,
> but it never gets queued more than once anyway. Also, calling
> queue_work() multiple times when the limit is hit could already happen
> today, so this isn't a significant change in any way.
>
> Signed-off-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx>
Looks good to me.
Reviewed-by: Chengming Zhou <chengming.zhou@xxxxxxxxx>
Thanks.
> ---
> mm/zswap.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index c4979c76d58e3..1cf3ab4b22e64 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1429,12 +1429,12 @@ bool zswap_store(struct folio *folio)
> if (cur_pages >= max_pages) {
> zswap_pool_limit_hit++;
> zswap_pool_reached_full = true;
> - goto shrink;
> + goto reject;
> }
>
> if (zswap_pool_reached_full) {
> if (cur_pages > zswap_accept_thr_pages())
> - goto shrink;
> + goto reject;
> else
> zswap_pool_reached_full = false;
> }
> @@ -1540,6 +1540,8 @@ bool zswap_store(struct folio *folio)
> zswap_entry_cache_free(entry);
> reject:
> obj_cgroup_put(objcg);
> + if (zswap_pool_reached_full)
> + queue_work(shrink_wq, &zswap_shrink_work);
> check_old:
> /*
> * If the zswap store fails or zswap is disabled, we must invalidate the
> @@ -1550,10 +1552,6 @@ bool zswap_store(struct folio *folio)
> if (entry)
> zswap_entry_free(entry);
> return false;
> -
> -shrink:
> - queue_work(shrink_wq, &zswap_shrink_work);
> - goto reject;
> }
>
> bool zswap_load(struct folio *folio)
