| Message ID | 20231206-zswap-lock-optimize-v1-4-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: > > Maybe I missed something, but the dstmem size of 2 * PAGE_SIZE is > very confusing, since we only need at most one page when compress, > and the "dlen" is also PAGE_SIZE in acomp_request_set_params(). > > So change it to one page, and fix the comments. > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > mm/zswap.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index d93a7b58b5af..999671dcb469 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -699,7 +699,7 @@ static int zswap_dstmem_prepare(unsigned int cpu) > struct mutex *mutex; > u8 *dst; > > - dst = kmalloc_node(PAGE_SIZE * 2, GFP_KERNEL, cpu_to_node(cpu)); > + dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > if (!dst) > return -ENOMEM; > > @@ -1649,8 +1649,7 @@ bool zswap_store(struct folio *folio) > sg_init_table(&input, 1); > sg_set_page(&input, page, PAGE_SIZE, 0); > > - /* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */ > - sg_init_one(&output, dst, PAGE_SIZE * 2); > + sg_init_one(&output, dst, PAGE_SIZE); Hmm. This is very weird. It looks very intentional though, so perhaps we should consult the maintainer or the original author of this logic to double check this? My best guess is for cases where the compression algorithm fails - i.e the output (header + payload) is somehow bigger than the original data. But not sure if this happens at all, and if the size > PAGE_SIZE we don't wanna store the output in zswap anyway. > acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen); > /* > * it maybe looks a little bit silly that we send an asynchronous request, > > -- > b4 0.10.1
On 2023/12/7 01:12, Nhat Pham wrote: > On Wed, Dec 6, 2023 at 1:46 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> Maybe I missed something, but the dstmem size of 2 * PAGE_SIZE is >> very confusing, since we only need at most one page when compress, >> and the "dlen" is also PAGE_SIZE in acomp_request_set_params(). >> >> So change it to one page, and fix the comments. >> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> mm/zswap.c | 5 ++--- >> 1 file changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/mm/zswap.c b/mm/zswap.c >> index d93a7b58b5af..999671dcb469 100644 >> --- a/mm/zswap.c >> +++ b/mm/zswap.c >> @@ -699,7 +699,7 @@ static int zswap_dstmem_prepare(unsigned int cpu) >> struct mutex *mutex; >> u8 *dst; >> >> - dst = kmalloc_node(PAGE_SIZE * 2, GFP_KERNEL, cpu_to_node(cpu)); >> + dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); >> if (!dst) >> return -ENOMEM; >> >> @@ -1649,8 +1649,7 @@ bool zswap_store(struct folio *folio) >> sg_init_table(&input, 1); >> sg_set_page(&input, page, PAGE_SIZE, 0); >> >> - /* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */ >> - sg_init_one(&output, dst, PAGE_SIZE * 2); >> + sg_init_one(&output, dst, PAGE_SIZE); > > Hmm. This is very weird. It looks very intentional though, so perhaps > we should consult the maintainer or the original author of this logic > to double check this? Yes, it's also weird to me. But it seems to be 2 pages when the zswap code merged in mm 10 years ago. Hope the maintainer or author could shed some light on it. :) > My best guess is for cases where the compression algorithm fails - i.e > the output (header + payload) is somehow bigger than the original > data. But not sure if this happens at all, and if the size > PAGE_SIZE > we don't wanna store the output in zswap anyway. > Agree, so I think one page is enough. >> acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen); And here we just use PAGE_SIZE in the parameter actually. Thanks! >> /* >> * it maybe looks a little bit silly that we send an asynchronous request, >> >> -- >> b4 0.10.1
diff --git a/mm/zswap.c b/mm/zswap.c index d93a7b58b5af..999671dcb469 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -699,7 +699,7 @@ static int zswap_dstmem_prepare(unsigned int cpu) struct mutex *mutex; u8 *dst; - dst = kmalloc_node(PAGE_SIZE * 2, GFP_KERNEL, cpu_to_node(cpu)); + dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); if (!dst) return -ENOMEM; @@ -1649,8 +1649,7 @@ bool zswap_store(struct folio *folio) sg_init_table(&input, 1); sg_set_page(&input, page, PAGE_SIZE, 0); - /* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */ - sg_init_one(&output, dst, PAGE_SIZE * 2); + sg_init_one(&output, dst, PAGE_SIZE); acomp_request_set_params(acomp_ctx->req, &input, &output, PAGE_SIZE, dlen); /* * it maybe looks a little bit silly that we send an asynchronous request,
Maybe I missed something, but the dstmem size of 2 * PAGE_SIZE is very confusing, since we only need at most one page when compress, and the "dlen" is also PAGE_SIZE in acomp_request_set_params(). So change it to one page, and fix the comments. Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> --- mm/zswap.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)