| Message ID | 20231206-zswap-lock-optimize-v1-5-e25b059f9c3a@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/zswap: optimize the scalability of zswap rb-tree | expand |
On Wed, Dec 6, 2023 at 1:46 AM Chengming Zhou <zhouchengming@bytedance.com> wrote: > > The zswap_load() and zswap_writeback_entry() have the same part that > decompress the data from zswap_entry to page, so refactor out the > common part as __zswap_load(entry, page). I love this refactoring a lot :) No reason why we should duplicate the decompression logic shared between load and writeback. > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > mm/zswap.c | 108 ++++++++++++++++++++++--------------------------------------- > 1 file changed, 39 insertions(+), 69 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 999671dcb469..667b66a3911b 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -1380,6 +1380,42 @@ static int zswap_enabled_param_set(const char *val, > return ret; > } > > +static void __zswap_load(struct zswap_entry *entry, struct page *page) > +{ > + struct scatterlist input, output; > + unsigned int dlen = PAGE_SIZE; > + struct crypto_acomp_ctx *acomp_ctx; > + struct zpool *zpool; > + u8 *src; > + int ret; > + > + /* decompress */ nit: I guess all this function does is decompression right? Doesn't seem like this comment is necessary anymore... But this is just nitpicking. > + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > + mutex_lock(acomp_ctx->mutex); > + > + zpool = zswap_find_zpool(entry); > + src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); > + if (!zpool_can_sleep_mapped(zpool)) { > + memcpy(acomp_ctx->dstmem, src, entry->length); > + src = acomp_ctx->dstmem; > + zpool_unmap_handle(zpool, entry->handle); > + } > + > + sg_init_one(&input, src, entry->length); > + sg_init_table(&output, 1); > + sg_set_page(&output, page, PAGE_SIZE, 0); > + acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); > + ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > + dlen = acomp_ctx->req->dlen; > + mutex_unlock(acomp_ctx->mutex); > + > + if (zpool_can_sleep_mapped(zpool)) > + zpool_unmap_handle(zpool, entry->handle); > + > + BUG_ON(ret); > + BUG_ON(dlen != PAGE_SIZE); > +} > + > /********************************* > * writeback code > **********************************/ > @@ -1401,23 +1437,12 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > swp_entry_t swpentry = entry->swpentry; > struct page *page; > struct mempolicy *mpol; > - struct scatterlist input, output; > - struct crypto_acomp_ctx *acomp_ctx; > - struct zpool *pool = zswap_find_zpool(entry); > bool page_was_allocated; > - u8 *src, *tmp = NULL; > - unsigned int dlen; > int ret; > struct writeback_control wbc = { > .sync_mode = WB_SYNC_NONE, > }; > > - if (!zpool_can_sleep_mapped(pool)) { > - tmp = kmalloc(PAGE_SIZE, GFP_KERNEL); > - if (!tmp) > - return -ENOMEM; > - } > - Sweet. Less allocation == more efficient + less failure case :) > /* try to allocate swap cache page */ > mpol = get_task_policy(current); > page = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, > @@ -1450,33 +1475,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > } > spin_unlock(&tree->lock); > > - /* decompress */ > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > - dlen = PAGE_SIZE; > - > - src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO); > - if (!zpool_can_sleep_mapped(pool)) { > - memcpy(tmp, src, entry->length); > - src = tmp; > - zpool_unmap_handle(pool, entry->handle); > - } > - > - mutex_lock(acomp_ctx->mutex); > - sg_init_one(&input, src, entry->length); > - sg_init_table(&output, 1); > - sg_set_page(&output, page, PAGE_SIZE, 0); > - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); > - ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > - dlen = acomp_ctx->req->dlen; > - mutex_unlock(acomp_ctx->mutex); > - > - if (!zpool_can_sleep_mapped(pool)) > - kfree(tmp); > - else > - zpool_unmap_handle(pool, entry->handle); > - > - BUG_ON(ret); > - BUG_ON(dlen != PAGE_SIZE); > + __zswap_load(entry, page); > > /* page is up to date */ > SetPageUptodate(page); > @@ -1496,9 +1495,6 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > return ret; > > fail: > - if (!zpool_can_sleep_mapped(pool)) > - kfree(tmp); > - > /* > * If we get here because the page is already in swapcache, a > * load may be happening concurrently. It is safe and okay to > @@ -1755,11 +1751,7 @@ bool zswap_load(struct folio *folio) > struct page *page = &folio->page; > struct zswap_tree *tree = swap_zswap_tree(swp); > struct zswap_entry *entry; > - struct scatterlist input, output; > - struct crypto_acomp_ctx *acomp_ctx; > - unsigned int dlen = PAGE_SIZE; > - u8 *src, *dst; > - struct zpool *zpool; > + u8 *dst; > bool ret; > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > @@ -1781,29 +1773,7 @@ bool zswap_load(struct folio *folio) > goto stats; > } > > - /* decompress */ > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > - mutex_lock(acomp_ctx->mutex); > - > - zpool = zswap_find_zpool(entry); > - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); > - if (!zpool_can_sleep_mapped(zpool)) { > - memcpy(acomp_ctx->dstmem, src, entry->length); > - src = acomp_ctx->dstmem; > - zpool_unmap_handle(zpool, entry->handle); > - } > - > - sg_init_one(&input, src, entry->length); > - sg_init_table(&output, 1); > - sg_set_page(&output, page, PAGE_SIZE, 0); > - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); > - if (crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)) > - WARN_ON(1); > - mutex_unlock(acomp_ctx->mutex); > - > - if (zpool_can_sleep_mapped(zpool)) > - zpool_unmap_handle(zpool, entry->handle); > - > + __zswap_load(entry, page); > ret = true; > stats: > count_vm_event(ZSWPIN); > > -- > b4 0.10.1 Can't find anything wrong with this patch, so: Reviewed-by: Nhat Pham <nphamcs@gmail.com>
On 2023/12/13 07:13, Nhat Pham wrote: > On Wed, Dec 6, 2023 at 1:46 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> The zswap_load() and zswap_writeback_entry() have the same part that >> decompress the data from zswap_entry to page, so refactor out the >> common part as __zswap_load(entry, page). > > I love this refactoring a lot :) No reason why we should duplicate the > decompression logic shared between load and writeback. > >> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> mm/zswap.c | 108 ++++++++++++++++++++++--------------------------------------- >> 1 file changed, 39 insertions(+), 69 deletions(-) >> >> diff --git a/mm/zswap.c b/mm/zswap.c >> index 999671dcb469..667b66a3911b 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -1380,6 +1380,42 @@ static int zswap_enabled_param_set(const char *val, >> return ret; >> } >> >> +static void __zswap_load(struct zswap_entry *entry, struct page *page) >> +{ >> + struct scatterlist input, output; >> + unsigned int dlen = PAGE_SIZE; >> + struct crypto_acomp_ctx *acomp_ctx; >> + struct zpool *zpool; >> + u8 *src; >> + int ret; >> + >> + /* decompress */ > > nit: I guess all this function does is decompression right? Doesn't > seem like this comment is necessary anymore... But this is just > nitpicking. > Ah, right. I will remove it. Thanks! >> + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); >> + mutex_lock(acomp_ctx->mutex); >> + >> + zpool = zswap_find_zpool(entry); >> + src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); >> + if (!zpool_can_sleep_mapped(zpool)) { >> + memcpy(acomp_ctx->dstmem, src, entry->length); >> + src = acomp_ctx->dstmem; >> + zpool_unmap_handle(zpool, entry->handle); >> + } >> + >> + sg_init_one(&input, src, entry->length); >> + sg_init_table(&output, 1); >> + sg_set_page(&output, page, PAGE_SIZE, 0); >> + acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); >> + ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); >> + dlen = acomp_ctx->req->dlen; >> + mutex_unlock(acomp_ctx->mutex); >> + >> + if (zpool_can_sleep_mapped(zpool)) >> + zpool_unmap_handle(zpool, entry->handle); >> + >> + BUG_ON(ret); >> + BUG_ON(dlen != PAGE_SIZE); >> +} >> + >> /********************************* >> * writeback code >> **********************************/ >> @@ -1401,23 +1437,12 @@ static int zswap_writeback_entry(struct zswap_entry *entry, >> swp_entry_t swpentry = entry->swpentry; >> struct page *page; >> struct mempolicy *mpol; >> - struct scatterlist input, output; >> - struct crypto_acomp_ctx *acomp_ctx; >> - struct zpool *pool = zswap_find_zpool(entry); >> bool page_was_allocated; >> - u8 *src, *tmp = NULL; >> - unsigned int dlen; >> int ret; >> struct writeback_control wbc = { >> .sync_mode = WB_SYNC_NONE, >> }; >> >> - if (!zpool_can_sleep_mapped(pool)) { >> - tmp = kmalloc(PAGE_SIZE, GFP_KERNEL); >> - if (!tmp) >> - return -ENOMEM; >> - } >> - > > Sweet. Less allocation == more efficient + less failure case :) > >> /* try to allocate swap cache page */ >> mpol = get_task_policy(current); >> page = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, >> @@ -1450,33 +1475,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, >> } >> spin_unlock(&tree->lock); >> >> - /* decompress */ >> - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); >> - dlen = PAGE_SIZE; >> - >> - src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO); >> - if (!zpool_can_sleep_mapped(pool)) { >> - memcpy(tmp, src, entry->length); >> - src = tmp; >> - zpool_unmap_handle(pool, entry->handle); >> - } >> - >> - mutex_lock(acomp_ctx->mutex); >> - sg_init_one(&input, src, entry->length); >> - sg_init_table(&output, 1); >> - sg_set_page(&output, page, PAGE_SIZE, 0); >> - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); >> - ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); >> - dlen = acomp_ctx->req->dlen; >> - mutex_unlock(acomp_ctx->mutex); >> - >> - if (!zpool_can_sleep_mapped(pool)) >> - kfree(tmp); >> - else >> - zpool_unmap_handle(pool, entry->handle); >> - >> - BUG_ON(ret); >> - BUG_ON(dlen != PAGE_SIZE); >> + __zswap_load(entry, page); >> >> /* page is up to date */ >> SetPageUptodate(page); >> @@ -1496,9 +1495,6 @@ static int zswap_writeback_entry(struct zswap_entry *entry, >> return ret; >> >> fail: >> - if (!zpool_can_sleep_mapped(pool)) >> - kfree(tmp); >> - >> /* >> * If we get here because the page is already in swapcache, a >> * load may be happening concurrently. It is safe and okay to >> @@ -1755,11 +1751,7 @@ bool zswap_load(struct folio *folio) >> struct page *page = &folio->page; >> struct zswap_tree *tree = swap_zswap_tree(swp); >> struct zswap_entry *entry; >> - struct scatterlist input, output; >> - struct crypto_acomp_ctx *acomp_ctx; >> - unsigned int dlen = PAGE_SIZE; >> - u8 *src, *dst; >> - struct zpool *zpool; >> + u8 *dst; >> bool ret; >> >> VM_WARN_ON_ONCE(!folio_test_locked(folio)); >> @@ -1781,29 +1773,7 @@ bool zswap_load(struct folio *folio) >> goto stats; >> } >> >> - /* decompress */ >> - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); >> - mutex_lock(acomp_ctx->mutex); >> - >> - zpool = zswap_find_zpool(entry); >> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); >> - if (!zpool_can_sleep_mapped(zpool)) { >> - memcpy(acomp_ctx->dstmem, src, entry->length); >> - src = acomp_ctx->dstmem; >> - zpool_unmap_handle(zpool, entry->handle); >> - } >> - >> - sg_init_one(&input, src, entry->length); >> - sg_init_table(&output, 1); >> - sg_set_page(&output, page, PAGE_SIZE, 0); >> - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); >> - if (crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)) >> - WARN_ON(1); >> - mutex_unlock(acomp_ctx->mutex); >> - >> - if (zpool_can_sleep_mapped(zpool)) >> - zpool_unmap_handle(zpool, entry->handle); >> - >> + __zswap_load(entry, page); >> ret = true; >> stats: >> count_vm_event(ZSWPIN); >> >> -- >> b4 0.10.1 > > Can't find anything wrong with this patch, so: > Reviewed-by: Nhat Pham <nphamcs@gmail.com>
diff --git a/mm/zswap.c b/mm/zswap.c index 999671dcb469..667b66a3911b 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -1380,6 +1380,42 @@ static int zswap_enabled_param_set(const char *val, return ret; } +static void __zswap_load(struct zswap_entry *entry, struct page *page) +{ + struct scatterlist input, output; + unsigned int dlen = PAGE_SIZE; + struct crypto_acomp_ctx *acomp_ctx; + struct zpool *zpool; + u8 *src; + int ret; + + /* decompress */ + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); + mutex_lock(acomp_ctx->mutex); + + zpool = zswap_find_zpool(entry); + src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); + if (!zpool_can_sleep_mapped(zpool)) { + memcpy(acomp_ctx->dstmem, src, entry->length); + src = acomp_ctx->dstmem; + zpool_unmap_handle(zpool, entry->handle); + } + + sg_init_one(&input, src, entry->length); + sg_init_table(&output, 1); + sg_set_page(&output, page, PAGE_SIZE, 0); + acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); + ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); + dlen = acomp_ctx->req->dlen; + mutex_unlock(acomp_ctx->mutex); + + if (zpool_can_sleep_mapped(zpool)) + zpool_unmap_handle(zpool, entry->handle); + + BUG_ON(ret); + BUG_ON(dlen != PAGE_SIZE); +} + /********************************* * writeback code **********************************/ @@ -1401,23 +1437,12 @@ static int zswap_writeback_entry(struct zswap_entry *entry, swp_entry_t swpentry = entry->swpentry; struct page *page; struct mempolicy *mpol; - struct scatterlist input, output; - struct crypto_acomp_ctx *acomp_ctx; - struct zpool *pool = zswap_find_zpool(entry); bool page_was_allocated; - u8 *src, *tmp = NULL; - unsigned int dlen; int ret; struct writeback_control wbc = { .sync_mode = WB_SYNC_NONE, }; - if (!zpool_can_sleep_mapped(pool)) { - tmp = kmalloc(PAGE_SIZE, GFP_KERNEL); - if (!tmp) - return -ENOMEM; - } - /* try to allocate swap cache page */ mpol = get_task_policy(current); page = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol, @@ -1450,33 +1475,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, } spin_unlock(&tree->lock); - /* decompress */ - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); - dlen = PAGE_SIZE; - - src = zpool_map_handle(pool, entry->handle, ZPOOL_MM_RO); - if (!zpool_can_sleep_mapped(pool)) { - memcpy(tmp, src, entry->length); - src = tmp; - zpool_unmap_handle(pool, entry->handle); - } - - mutex_lock(acomp_ctx->mutex); - sg_init_one(&input, src, entry->length); - sg_init_table(&output, 1); - sg_set_page(&output, page, PAGE_SIZE, 0); - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); - ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); - dlen = acomp_ctx->req->dlen; - mutex_unlock(acomp_ctx->mutex); - - if (!zpool_can_sleep_mapped(pool)) - kfree(tmp); - else - zpool_unmap_handle(pool, entry->handle); - - BUG_ON(ret); - BUG_ON(dlen != PAGE_SIZE); + __zswap_load(entry, page); /* page is up to date */ SetPageUptodate(page); @@ -1496,9 +1495,6 @@ static int zswap_writeback_entry(struct zswap_entry *entry, return ret; fail: - if (!zpool_can_sleep_mapped(pool)) - kfree(tmp); - /* * If we get here because the page is already in swapcache, a * load may be happening concurrently. It is safe and okay to @@ -1755,11 +1751,7 @@ bool zswap_load(struct folio *folio) struct page *page = &folio->page; struct zswap_tree *tree = swap_zswap_tree(swp); struct zswap_entry *entry; - struct scatterlist input, output; - struct crypto_acomp_ctx *acomp_ctx; - unsigned int dlen = PAGE_SIZE; - u8 *src, *dst; - struct zpool *zpool; + u8 *dst; bool ret; VM_WARN_ON_ONCE(!folio_test_locked(folio)); @@ -1781,29 +1773,7 @@ bool zswap_load(struct folio *folio) goto stats; } - /* decompress */ - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); - mutex_lock(acomp_ctx->mutex); - - zpool = zswap_find_zpool(entry); - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); - if (!zpool_can_sleep_mapped(zpool)) { - memcpy(acomp_ctx->dstmem, src, entry->length); - src = acomp_ctx->dstmem; - zpool_unmap_handle(zpool, entry->handle); - } - - sg_init_one(&input, src, entry->length); - sg_init_table(&output, 1); - sg_set_page(&output, page, PAGE_SIZE, 0); - acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dlen); - if (crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)) - WARN_ON(1); - mutex_unlock(acomp_ctx->mutex); - - if (zpool_can_sleep_mapped(zpool)) - zpool_unmap_handle(zpool, entry->handle); - + __zswap_load(entry, page); ret = true; stats: count_vm_event(ZSWPIN);
The zswap_load() and zswap_writeback_entry() have the same part that decompress the data from zswap_entry to page, so refactor out the common part as __zswap_load(entry, page). Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> --- mm/zswap.c | 108 ++++++++++++++++++++++--------------------------------------- 1 file changed, 39 insertions(+), 69 deletions(-)