| Message ID | 20240325235018.2028408-2-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | zswap: store zero-filled pages more efficiently | expand |
On Mon, Mar 25, 2024 at 4:50 PM Yosry Ahmed <yosryahmed@google.com> 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. Yup. > - 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. Agree. > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> This change by itself seems fine to me. Reviewed-by: Nhat Pham <nphamcs@gmail.com>
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@google.com> Looks good to me. Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev> 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)
On Mon, Mar 25, 2024 at 11:50:09PM +0000, 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@google.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
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)
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@google.com> --- mm/zswap.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)