| Message ID | 20231213-zswap-dstmem-v3-6-4eac09b94ece@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/zswap: dstmem reuse optimizations and cleanups | expand |
Hi Chengming and Yosry, On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou <zhouchengming@bytedance.com> wrote: > > Since the introduce of reusing the dstmem in the load path, it seems > confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex > now for purposes other than what the naming suggests. > > Yosry suggested removing these two fields from acomp_ctx, and directly > using zswap_dstmem and zswap_mutex in both the load and store paths, > rename them, and add proper comments above their definitions that they > are for generic percpu buffering on the load and store paths. > > So this patch remove dstmem and mutex from acomp_ctx, and rename the > zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on > the load and store paths. Sorry joining this discussion late. I get the rename of "dstmem" to "buffer". Because the buffer is used for both load and store as well. What I don't get is that, why do we move it out of the acomp_ctx struct. Now we have 3 per cpu entry: buffer, mutex and acomp_ctx. I think we should do the reverse, fold this three per cpu entry into one struct the acomp_ctx. Each per_cpu load() has a sequence of dance around the cpu id and disable preempt etc, while each of the struct member load is just a plan memory load. It seems to me it would be more optimal to combine this three per cpu entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. > > Suggested-by: Yosry Ahmed <yosryahmed@google.com> > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > mm/zswap.c | 69 +++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 37 insertions(+), 32 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index 2c349fd88904..71bdcd552e5b 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -166,8 +166,6 @@ struct crypto_acomp_ctx { > struct crypto_acomp *acomp; > struct acomp_req *req; > struct crypto_wait wait; > - u8 *dstmem; > - struct mutex *mutex; > }; > > /* > @@ -694,7 +692,7 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) > /********************************* > * per-cpu code > **********************************/ > -static DEFINE_PER_CPU(u8 *, zswap_dstmem); > +static DEFINE_PER_CPU(u8 *, zswap_buffer); > /* > * If users dynamically change the zpool type and compressor at runtime, i.e. > * zswap is running, zswap can have more than one zpool on one cpu, but they > @@ -702,39 +700,39 @@ static DEFINE_PER_CPU(u8 *, zswap_dstmem); > */ > static DEFINE_PER_CPU(struct mutex *, zswap_mutex); > > -static int zswap_dstmem_prepare(unsigned int cpu) > +static int zswap_buffer_prepare(unsigned int cpu) > { > struct mutex *mutex; > - u8 *dst; > + u8 *buf; > > - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > - if (!dst) > + buf = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > + if (!buf) > return -ENOMEM; > > mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); > if (!mutex) { > - kfree(dst); > + kfree(buf); > return -ENOMEM; > } > > mutex_init(mutex); > - per_cpu(zswap_dstmem, cpu) = dst; > + per_cpu(zswap_buffer, cpu) = buf; > per_cpu(zswap_mutex, cpu) = mutex; > return 0; > } > > -static int zswap_dstmem_dead(unsigned int cpu) > +static int zswap_buffer_dead(unsigned int cpu) > { > struct mutex *mutex; > - u8 *dst; > + u8 *buf; > > mutex = per_cpu(zswap_mutex, cpu); > kfree(mutex); > per_cpu(zswap_mutex, cpu) = NULL; > > - dst = per_cpu(zswap_dstmem, cpu); > - kfree(dst); > - per_cpu(zswap_dstmem, cpu) = NULL; > + buf = per_cpu(zswap_buffer, cpu); > + kfree(buf); > + per_cpu(zswap_buffer, cpu) = NULL; > > return 0; > } > @@ -772,9 +770,6 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, > crypto_req_done, &acomp_ctx->wait); > > - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); > - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); > - > return 0; > } > > @@ -1397,15 +1392,21 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) > struct zpool *zpool = zswap_find_zpool(entry); > struct scatterlist input, output; > struct crypto_acomp_ctx *acomp_ctx; > - u8 *src; > + u8 *src, *buf; > + int cpu; > + struct mutex *mutex; > > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > - mutex_lock(acomp_ctx->mutex); > + cpu = raw_smp_processor_id(); > + mutex = per_cpu(zswap_mutex, cpu); First per cpu call. > + mutex_lock(mutex); > + > + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); Second per cpu > > 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; > + buf = per_cpu(zswap_buffer, cpu); Here is the function that does the third per_cpu call. I think doing one per_cpu and the rest of them load from the context is more optimal. Conceptually it is cleaner as well. It is clear what this mutex is supposed to protect. It is protecting the compression context struct. Move it out as per cpu, it is less clear what is the protection scope of the mutex. What am I missing? Chris > + memcpy(buf, src, entry->length); > + src = buf; > zpool_unmap_handle(zpool, entry->handle); > } > > @@ -1415,7 +1416,7 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) > acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, PAGE_SIZE); > BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)); > BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE); > - mutex_unlock(acomp_ctx->mutex); > + mutex_unlock(mutex); > > if (zpool_can_sleep_mapped(zpool)) > zpool_unmap_handle(zpool, entry->handle); > @@ -1551,6 +1552,8 @@ bool zswap_store(struct folio *folio) > u8 *src, *dst; > gfp_t gfp; > int ret; > + int cpu; > + struct mutex *mutex; > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); > @@ -1636,11 +1639,13 @@ bool zswap_store(struct folio *folio) > } > > /* compress */ > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > + cpu = raw_smp_processor_id(); > + mutex = per_cpu(zswap_mutex, cpu); > + mutex_lock(mutex); > > - mutex_lock(acomp_ctx->mutex); > + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); > + dst = per_cpu(zswap_buffer, cpu); > > - dst = acomp_ctx->dstmem; > sg_init_table(&input, 1); > sg_set_page(&input, page, PAGE_SIZE, 0); > > @@ -1683,7 +1688,7 @@ bool zswap_store(struct folio *folio) > buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO); > memcpy(buf, dst, dlen); > zpool_unmap_handle(zpool, handle); > - mutex_unlock(acomp_ctx->mutex); > + mutex_unlock(mutex); > > /* populate entry */ > entry->swpentry = swp_entry(type, offset); > @@ -1726,7 +1731,7 @@ bool zswap_store(struct folio *folio) > return true; > > put_dstmem: > - mutex_unlock(acomp_ctx->mutex); > + mutex_unlock(mutex); > put_pool: > zswap_pool_put(entry->pool); > freepage: > @@ -1902,10 +1907,10 @@ static int zswap_setup(void) > } > > ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", > - zswap_dstmem_prepare, zswap_dstmem_dead); > + zswap_buffer_prepare, zswap_buffer_dead); > if (ret) { > - pr_err("dstmem alloc failed\n"); > - goto dstmem_fail; > + pr_err("buffer alloc failed\n"); > + goto buffer_fail; > } > > ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, > @@ -1940,7 +1945,7 @@ static int zswap_setup(void) > zswap_pool_destroy(pool); > hp_fail: > cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); > -dstmem_fail: > +buffer_fail: > kmem_cache_destroy(zswap_entry_cache); > cache_fail: > /* if built-in, we aren't unloaded on failure; don't allow use */ > > -- > b4 0.10.1
On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: > > Hi Chengming and Yosry, > > On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: > > > > Since the introduce of reusing the dstmem in the load path, it seems > > confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex > > now for purposes other than what the naming suggests. > > > > Yosry suggested removing these two fields from acomp_ctx, and directly > > using zswap_dstmem and zswap_mutex in both the load and store paths, > > rename them, and add proper comments above their definitions that they > > are for generic percpu buffering on the load and store paths. > > > > So this patch remove dstmem and mutex from acomp_ctx, and rename the > > zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on > > the load and store paths. > > Sorry joining this discussion late. > > I get the rename of "dstmem" to "buffer". Because the buffer is used > for both load and store as well. What I don't get is that, why do we > move it out of the acomp_ctx struct. Now we have 3 per cpu entry: > buffer, mutex and acomp_ctx. I think we should do the reverse, fold > this three per cpu entry into one struct the acomp_ctx. Each per_cpu > load() has a sequence of dance around the cpu id and disable preempt > etc, while each of the struct member load is just a plan memory load. > It seems to me it would be more optimal to combine this three per cpu > entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. I agree with Chris. From a practicality POV, what Chris says here makes sense. From a semantic POV, this buffer is only used in (de)compression contexts - be it in store, load, or writeback - so it belonging to the orignal struct still makes sense to me. Why separate it out, without any benefits. Just rename the old field buffer or zswap_buffer and call it a day? It will be a smaller patch too! > > > > > Suggested-by: Yosry Ahmed <yosryahmed@google.com> > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > > --- > > mm/zswap.c | 69 +++++++++++++++++++++++++++++++++----------------------------- > > 1 file changed, 37 insertions(+), 32 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 2c349fd88904..71bdcd552e5b 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -166,8 +166,6 @@ struct crypto_acomp_ctx { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > struct crypto_wait wait; > > - u8 *dstmem; > > - struct mutex *mutex; > > }; > > > > /* > > @@ -694,7 +692,7 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) > > /********************************* > > * per-cpu code > > **********************************/ > > -static DEFINE_PER_CPU(u8 *, zswap_dstmem); > > +static DEFINE_PER_CPU(u8 *, zswap_buffer); > > /* > > * If users dynamically change the zpool type and compressor at runtime, i.e. > > * zswap is running, zswap can have more than one zpool on one cpu, but they > > @@ -702,39 +700,39 @@ static DEFINE_PER_CPU(u8 *, zswap_dstmem); > > */ > > static DEFINE_PER_CPU(struct mutex *, zswap_mutex); > > > > -static int zswap_dstmem_prepare(unsigned int cpu) > > +static int zswap_buffer_prepare(unsigned int cpu) > > { > > struct mutex *mutex; > > - u8 *dst; > > + u8 *buf; > > > > - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > > - if (!dst) > > + buf = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > > + if (!buf) > > return -ENOMEM; > > > > mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); > > if (!mutex) { > > - kfree(dst); > > + kfree(buf); > > return -ENOMEM; > > } > > > > mutex_init(mutex); > > - per_cpu(zswap_dstmem, cpu) = dst; > > + per_cpu(zswap_buffer, cpu) = buf; > > per_cpu(zswap_mutex, cpu) = mutex; > > return 0; > > } > > > > -static int zswap_dstmem_dead(unsigned int cpu) > > +static int zswap_buffer_dead(unsigned int cpu) > > { > > struct mutex *mutex; > > - u8 *dst; > > + u8 *buf; > > > > mutex = per_cpu(zswap_mutex, cpu); > > kfree(mutex); > > per_cpu(zswap_mutex, cpu) = NULL; > > > > - dst = per_cpu(zswap_dstmem, cpu); > > - kfree(dst); > > - per_cpu(zswap_dstmem, cpu) = NULL; > > + buf = per_cpu(zswap_buffer, cpu); > > + kfree(buf); > > + per_cpu(zswap_buffer, cpu) = NULL; > > > > return 0; > > } > > @@ -772,9 +770,6 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > > acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, > > crypto_req_done, &acomp_ctx->wait); > > > > - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); > > - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); > > - > > return 0; > > } > > > > @@ -1397,15 +1392,21 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) > > struct zpool *zpool = zswap_find_zpool(entry); > > struct scatterlist input, output; > > struct crypto_acomp_ctx *acomp_ctx; > > - u8 *src; > > + u8 *src, *buf; > > + int cpu; > > + struct mutex *mutex; > > > > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > > - mutex_lock(acomp_ctx->mutex); > > + cpu = raw_smp_processor_id(); > > + mutex = per_cpu(zswap_mutex, cpu); > First per cpu call. > > + mutex_lock(mutex); > > + > > + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); > Second per cpu > > > > 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; > > + buf = per_cpu(zswap_buffer, cpu); > > Here is the function that does the third per_cpu call. I think doing > one per_cpu and the rest of them load from the context is more > optimal. > Conceptually it is cleaner as well. It is clear what this mutex is > supposed to protect. It is protecting the compression context struct. > Move it out as per cpu, it is less clear what is the protection scope > of the mutex. > > What am I missing? > > Chris > > > > + memcpy(buf, src, entry->length); > > + src = buf; > > zpool_unmap_handle(zpool, entry->handle); > > } > > > > @@ -1415,7 +1416,7 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) > > acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, PAGE_SIZE); > > BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)); > > BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE); > > - mutex_unlock(acomp_ctx->mutex); > > + mutex_unlock(mutex); > > > > if (zpool_can_sleep_mapped(zpool)) > > zpool_unmap_handle(zpool, entry->handle); > > @@ -1551,6 +1552,8 @@ bool zswap_store(struct folio *folio) > > u8 *src, *dst; > > gfp_t gfp; > > int ret; > > + int cpu; > > + struct mutex *mutex; > > > > VM_WARN_ON_ONCE(!folio_test_locked(folio)); > > VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); > > @@ -1636,11 +1639,13 @@ bool zswap_store(struct folio *folio) > > } > > > > /* compress */ > > - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); > > + cpu = raw_smp_processor_id(); > > + mutex = per_cpu(zswap_mutex, cpu); > > + mutex_lock(mutex); > > > > - mutex_lock(acomp_ctx->mutex); > > + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); > > + dst = per_cpu(zswap_buffer, cpu); > > > > - dst = acomp_ctx->dstmem; > > sg_init_table(&input, 1); > > sg_set_page(&input, page, PAGE_SIZE, 0); > > > > @@ -1683,7 +1688,7 @@ bool zswap_store(struct folio *folio) > > buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO); > > memcpy(buf, dst, dlen); > > zpool_unmap_handle(zpool, handle); > > - mutex_unlock(acomp_ctx->mutex); > > + mutex_unlock(mutex); > > > > /* populate entry */ > > entry->swpentry = swp_entry(type, offset); > > @@ -1726,7 +1731,7 @@ bool zswap_store(struct folio *folio) > > return true; > > > > put_dstmem: > > - mutex_unlock(acomp_ctx->mutex); > > + mutex_unlock(mutex); > > put_pool: > > zswap_pool_put(entry->pool); > > freepage: > > @@ -1902,10 +1907,10 @@ static int zswap_setup(void) > > } > > > > ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", > > - zswap_dstmem_prepare, zswap_dstmem_dead); > > + zswap_buffer_prepare, zswap_buffer_dead); > > if (ret) { > > - pr_err("dstmem alloc failed\n"); > > - goto dstmem_fail; > > + pr_err("buffer alloc failed\n"); > > + goto buffer_fail; > > } > > > > ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, > > @@ -1940,7 +1945,7 @@ static int zswap_setup(void) > > zswap_pool_destroy(pool); > > hp_fail: > > cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); > > -dstmem_fail: > > +buffer_fail: > > kmem_cache_destroy(zswap_entry_cache); > > cache_fail: > > /* if built-in, we aren't unloaded on failure; don't allow use */ > > > > -- > > b4 0.10.1
On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote: > > On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: > > > > Hi Chengming and Yosry, > > > > On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou > > <zhouchengming@bytedance.com> wrote: > > > > > > Since the introduce of reusing the dstmem in the load path, it seems > > > confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex > > > now for purposes other than what the naming suggests. > > > > > > Yosry suggested removing these two fields from acomp_ctx, and directly > > > using zswap_dstmem and zswap_mutex in both the load and store paths, > > > rename them, and add proper comments above their definitions that they > > > are for generic percpu buffering on the load and store paths. > > > > > > So this patch remove dstmem and mutex from acomp_ctx, and rename the > > > zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on > > > the load and store paths. > > > > Sorry joining this discussion late. > > > > I get the rename of "dstmem" to "buffer". Because the buffer is used > > for both load and store as well. What I don't get is that, why do we > > move it out of the acomp_ctx struct. Now we have 3 per cpu entry: > > buffer, mutex and acomp_ctx. I think we should do the reverse, fold > > this three per cpu entry into one struct the acomp_ctx. Each per_cpu > > load() has a sequence of dance around the cpu id and disable preempt > > etc, while each of the struct member load is just a plan memory load. > > It seems to me it would be more optimal to combine this three per cpu > > entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. > > I agree with Chris. From a practicality POV, what Chris says here > makes sense. From a semantic POV, this buffer is only used in > (de)compression contexts - be it in store, load, or writeback - so it > belonging to the orignal struct still makes sense to me. Why separate > it out, without any benefits. Just rename the old field buffer or > zswap_buffer and call it a day? It will be a smaller patch too! > My main concern is that the struct name is specific for the crypto acomp stuff, but that buffer and mutex are not. How about we keep it in the struct, but refactor the struct as follows: struct zswap_ctx { struct { struct crypto_acomp *acomp; struct acomp_req *req; struct crypto_wait wait; } acomp_ctx; u8 *dstmem; struct mutex *mutex; }; , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx?
Hi Yosry, On Tue, Dec 19, 2023 at 1:39 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > My main concern is that the struct name is specific for the crypto > acomp stuff, but that buffer and mutex are not. > How about we keep it in the struct, but refactor the struct as follows: If it is the naming of the struct you are not happy about. We can change the naming. > > struct zswap_ctx { > struct { > struct crypto_acomp *acomp; > struct acomp_req *req; > struct crypto_wait wait; > } acomp_ctx; > u8 *dstmem; > struct mutex *mutex; > }; The compression and decompression requires the buffer and mutex. The mutex is not used other than compress and decompress, right? In my mind, they are fine staying in the struct. I am not sure adding an level acomp_ctx provides anything. It makes access structure members deeper. If you care about separating out the crypto acomp, how about just remove acomp_ctx and make it an anonymous structure. struct zswap_comp_ctx { struct /* cryto acomp context */ { struct crypto_acomp *acomp; struct acomp_req *req; struct crypto_wait wait; }; u8 *dstmem; struct mutex *mutex; }; Then we remove other per_cpu_load as well. I also think the original struct name is fine, we don't need to change the struct name. The value/code_change ratio for renaming the struct alone is low. On the other hand, if we can get rid of some per cpu load, that value is high enough to justify the patch. Chris
On Tue, Dec 19, 2023 at 2:49 PM Chris Li <chrisl@kernel.org> wrote: > > Hi Yosry, > > On Tue, Dec 19, 2023 at 1:39 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > > > My main concern is that the struct name is specific for the crypto > > acomp stuff, but that buffer and mutex are not. > > How about we keep it in the struct, but refactor the struct as follows: > > If it is the naming of the struct you are not happy about. We can > change the naming. > > > > > struct zswap_ctx { > > struct { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > struct crypto_wait wait; > > } acomp_ctx; > > u8 *dstmem; > > struct mutex *mutex; > > }; > > The compression and decompression requires the buffer and mutex. The > mutex is not used other than compress and decompress, right? > In my mind, they are fine staying in the struct. I am not sure adding > an level acomp_ctx provides anything. It makes access structure > members deeper. > > If you care about separating out the crypto acomp, how about just > remove acomp_ctx and make it an anonymous structure. > struct zswap_comp_ctx { > struct /* cryto acomp context */ { > struct crypto_acomp *acomp; > struct acomp_req *req; > struct crypto_wait wait; > }; > u8 *dstmem; > struct mutex *mutex; > }; I prefer naming the internal struct, but I am fine with an anonymous struct as well. I just think it's a semantically sound separation. > > Then we remove other per_cpu_load as well. > > I also think the original struct name is fine, we don't need to change > the struct name. The original struct name makes it seems like the data in the struct is only used for the crypto acomp API, which is not the case.
Hi Yosry, On Tue, Dec 19, 2023 at 3:05 PM Yosry Ahmed <yosryahmed@google.com> wrote: > > The compression and decompression requires the buffer and mutex. The > > mutex is not used other than compress and decompress, right? > > In my mind, they are fine staying in the struct. I am not sure adding > > an level acomp_ctx provides anything. It makes access structure > > members deeper. > > > > If you care about separating out the crypto acomp, how about just > > remove acomp_ctx and make it an anonymous structure. > > struct zswap_comp_ctx { > > struct /* cryto acomp context */ { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > struct crypto_wait wait; > > }; > > u8 *dstmem; > > struct mutex *mutex; > > }; > > I prefer naming the internal struct, but I am fine with an anonymous > struct as well. I just think it's a semantically sound separation. Ack. > > > > > Then we remove other per_cpu_load as well. > > > > I also think the original struct name is fine, we don't need to change > > the struct name. > > The original struct name makes it seems like the data in the struct is > only used for the crypto acomp API, which is not the case. The mutex and buffer are used associated with the crypto acomp API that is why I think it is fine to stay within the struct as well. Using the anonymous struct to separate it out is marginally better. I think we are in agreement here. Chris
On 2023/12/20 05:39, Yosry Ahmed wrote: > On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote: >> >> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: >>> >>> Hi Chengming and Yosry, >>> >>> On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou >>> <zhouchengming@bytedance.com> wrote: >>>> >>>> Since the introduce of reusing the dstmem in the load path, it seems >>>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex >>>> now for purposes other than what the naming suggests. >>>> >>>> Yosry suggested removing these two fields from acomp_ctx, and directly >>>> using zswap_dstmem and zswap_mutex in both the load and store paths, >>>> rename them, and add proper comments above their definitions that they >>>> are for generic percpu buffering on the load and store paths. >>>> >>>> So this patch remove dstmem and mutex from acomp_ctx, and rename the >>>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on >>>> the load and store paths. >>> >>> Sorry joining this discussion late. >>> >>> I get the rename of "dstmem" to "buffer". Because the buffer is used >>> for both load and store as well. What I don't get is that, why do we >>> move it out of the acomp_ctx struct. Now we have 3 per cpu entry: >>> buffer, mutex and acomp_ctx. I think we should do the reverse, fold >>> this three per cpu entry into one struct the acomp_ctx. Each per_cpu >>> load() has a sequence of dance around the cpu id and disable preempt >>> etc, while each of the struct member load is just a plan memory load. >>> It seems to me it would be more optimal to combine this three per cpu >>> entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. >> >> I agree with Chris. From a practicality POV, what Chris says here >> makes sense. From a semantic POV, this buffer is only used in >> (de)compression contexts - be it in store, load, or writeback - so it >> belonging to the orignal struct still makes sense to me. Why separate >> it out, without any benefits. Just rename the old field buffer or >> zswap_buffer and call it a day? It will be a smaller patch too! >> > > My main concern is that the struct name is specific for the crypto > acomp stuff, but that buffer and mutex are not. > How about we keep it in the struct, but refactor the struct as follows: > > struct zswap_ctx { > struct { > struct crypto_acomp *acomp; > struct acomp_req *req; > struct crypto_wait wait; > } acomp_ctx; > u8 *dstmem; > struct mutex *mutex; > }; > > , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx? I think there are two viewpoints here, both works ok to me. The first is from ownship or lifetime, these percpu mutex and dstmem are shared between all pools, so no one pool own the mutex and dstmem, nor does the percpu acomp_ctx in each pool. The second is from usage, these percpu mutex and dstmem are used by the percpu acomp_ctx in each pool, so it's easy to use to put pointers to them in the percpu acomp_ctx. Actually I think it's simpler to let the percpu acomp_ctx has its own mutex and dstmem, which in fact are the necessary parts when it use the acomp interfaces. This way, we could delete the percpu mutex and dstmem, and its hotplugs, and not shared anymore between all pools. Maybe we would have many pools at the same time in the future, like different compression algorithm or different zpool for different memcg workloads. Who knows? :-) So how about this patch below? Just RFC. Subject: [PATCH] mm/zswap: make each acomp_ctx has its own mutex and dstmem Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> --- include/linux/cpuhotplug.h | 1 - mm/zswap.c | 86 ++++++++++++-------------------------- 2 files changed, 26 insertions(+), 61 deletions(-) diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h index efc0c0b07efb..c3e06e21766a 100644 --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -124,7 +124,6 @@ enum cpuhp_state { CPUHP_ARM_BL_PREPARE, CPUHP_TRACE_RB_PREPARE, CPUHP_MM_ZS_PREPARE, - CPUHP_MM_ZSWP_MEM_PREPARE, CPUHP_MM_ZSWP_POOL_PREPARE, CPUHP_KVM_PPC_BOOK3S_PREPARE, CPUHP_ZCOMP_PREPARE, diff --git a/mm/zswap.c b/mm/zswap.c index 2c349fd88904..37301f1a80a9 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -694,63 +694,31 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) /********************************* * per-cpu code **********************************/ -static DEFINE_PER_CPU(u8 *, zswap_dstmem); -/* - * If users dynamically change the zpool type and compressor at runtime, i.e. - * zswap is running, zswap can have more than one zpool on one cpu, but they - * are sharing dtsmem. So we need this mutex to be per-cpu. - */ -static DEFINE_PER_CPU(struct mutex *, zswap_mutex); - -static int zswap_dstmem_prepare(unsigned int cpu) -{ - struct mutex *mutex; - u8 *dst; - - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); - if (!dst) - return -ENOMEM; - - mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); - if (!mutex) { - kfree(dst); - return -ENOMEM; - } - - mutex_init(mutex); - per_cpu(zswap_dstmem, cpu) = dst; - per_cpu(zswap_mutex, cpu) = mutex; - return 0; -} - -static int zswap_dstmem_dead(unsigned int cpu) -{ - struct mutex *mutex; - u8 *dst; - - mutex = per_cpu(zswap_mutex, cpu); - kfree(mutex); - per_cpu(zswap_mutex, cpu) = NULL; - - dst = per_cpu(zswap_dstmem, cpu); - kfree(dst); - per_cpu(zswap_dstmem, cpu) = NULL; - - return 0; -} - static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) { struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node); struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu); struct crypto_acomp *acomp; struct acomp_req *req; + int ret = 0; + + acomp_ctx->dstmem = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); + if (!acomp_ctx->dstmem) + return -ENOMEM; + + acomp_ctx->mutex = kmalloc_node(sizeof(struct mutex), GFP_KERNEL, cpu_to_node(cpu)); + if (!acomp_ctx->mutex) { + ret = -ENOMEM; + goto mutex_fail; + } + mutex_init(acomp_ctx->mutex); acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu)); if (IS_ERR(acomp)) { pr_err("could not alloc crypto acomp %s : %ld\n", pool->tfm_name, PTR_ERR(acomp)); - return PTR_ERR(acomp); + ret = PTR_ERR(acomp); + goto acomp_fail; } acomp_ctx->acomp = acomp; @@ -758,8 +726,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) if (!req) { pr_err("could not alloc crypto acomp_request %s\n", pool->tfm_name); - crypto_free_acomp(acomp_ctx->acomp); - return -ENOMEM; + ret = -ENOMEM; + goto req_fail; } acomp_ctx->req = req; @@ -772,10 +740,15 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, crypto_req_done, &acomp_ctx->wait); - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); - return 0; +req_fail: + crypto_free_acomp(acomp_ctx->acomp); +acomp_fail: + kfree(acomp_ctx->mutex); +mutex_fail: + kfree(acomp_ctx->dstmem); + + return ret; } static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) @@ -788,6 +761,8 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) acomp_request_free(acomp_ctx->req); if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) crypto_free_acomp(acomp_ctx->acomp); + kfree(acomp_ctx->mutex); + kfree(acomp_ctx->dstmem); } return 0; @@ -1901,13 +1876,6 @@ static int zswap_setup(void) goto cache_fail; } - ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", - zswap_dstmem_prepare, zswap_dstmem_dead); - if (ret) { - pr_err("dstmem alloc failed\n"); - goto dstmem_fail; - } - ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, "mm/zswap_pool:prepare", zswap_cpu_comp_prepare, @@ -1939,8 +1907,6 @@ static int zswap_setup(void) if (pool) zswap_pool_destroy(pool); hp_fail: - cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); -dstmem_fail: kmem_cache_destroy(zswap_entry_cache); cache_fail: /* if built-in, we aren't unloaded on failure; don't allow use */ -- 2.20.1
On Wed, Dec 20, 2023 at 4:20 AM Chengming Zhou <zhouchengming@bytedance.com> wrote: > > On 2023/12/20 05:39, Yosry Ahmed wrote: > > On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote: > >> > >> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: > >>> > >>> Hi Chengming and Yosry, > >>> > >>> On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou > >>> <zhouchengming@bytedance.com> wrote: > >>>> > >>>> Since the introduce of reusing the dstmem in the load path, it seems > >>>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex > >>>> now for purposes other than what the naming suggests. > >>>> > >>>> Yosry suggested removing these two fields from acomp_ctx, and directly > >>>> using zswap_dstmem and zswap_mutex in both the load and store paths, > >>>> rename them, and add proper comments above their definitions that they > >>>> are for generic percpu buffering on the load and store paths. > >>>> > >>>> So this patch remove dstmem and mutex from acomp_ctx, and rename the > >>>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on > >>>> the load and store paths. > >>> > >>> Sorry joining this discussion late. > >>> > >>> I get the rename of "dstmem" to "buffer". Because the buffer is used > >>> for both load and store as well. What I don't get is that, why do we > >>> move it out of the acomp_ctx struct. Now we have 3 per cpu entry: > >>> buffer, mutex and acomp_ctx. I think we should do the reverse, fold > >>> this three per cpu entry into one struct the acomp_ctx. Each per_cpu > >>> load() has a sequence of dance around the cpu id and disable preempt > >>> etc, while each of the struct member load is just a plan memory load. > >>> It seems to me it would be more optimal to combine this three per cpu > >>> entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. > >> > >> I agree with Chris. From a practicality POV, what Chris says here > >> makes sense. From a semantic POV, this buffer is only used in > >> (de)compression contexts - be it in store, load, or writeback - so it > >> belonging to the orignal struct still makes sense to me. Why separate > >> it out, without any benefits. Just rename the old field buffer or > >> zswap_buffer and call it a day? It will be a smaller patch too! > >> > > > > My main concern is that the struct name is specific for the crypto > > acomp stuff, but that buffer and mutex are not. > > How about we keep it in the struct, but refactor the struct as follows: > > > > struct zswap_ctx { > > struct { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > struct crypto_wait wait; > > } acomp_ctx; > > u8 *dstmem; > > struct mutex *mutex; > > }; > > > > , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx? > > I think there are two viewpoints here, both works ok to me. > > The first is from ownship or lifetime, these percpu mutex and dstmem > are shared between all pools, so no one pool own the mutex and dstmem, > nor does the percpu acomp_ctx in each pool. > > The second is from usage, these percpu mutex and dstmem are used by > the percpu acomp_ctx in each pool, so it's easy to use to put pointers > to them in the percpu acomp_ctx. > > Actually I think it's simpler to let the percpu acomp_ctx has its own > mutex and dstmem, which in fact are the necessary parts when it use > the acomp interfaces. > > This way, we could delete the percpu mutex and dstmem, and its hotplugs, > and not shared anymore between all pools. Maybe we would have many pools > at the same time in the future, like different compression algorithm or > different zpool for different memcg workloads. Who knows? :-) > > So how about this patch below? Just RFC. The general approach looks fine to me, although I still prefer we reorganize the struct as Chris and I discussed: rename crypto_acomp_ctx to a generic name, add a (anonymous) struct for the crypto_acomp stuff, rename dstmem to buffer or so. I think we can also make the mutex a static part of the struct, any advantage to dynamically allocating it? > > Subject: [PATCH] mm/zswap: make each acomp_ctx has its own mutex and dstmem > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > include/linux/cpuhotplug.h | 1 - > mm/zswap.c | 86 ++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 61 deletions(-) > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > index efc0c0b07efb..c3e06e21766a 100644 > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -124,7 +124,6 @@ enum cpuhp_state { > CPUHP_ARM_BL_PREPARE, > CPUHP_TRACE_RB_PREPARE, > CPUHP_MM_ZS_PREPARE, > - CPUHP_MM_ZSWP_MEM_PREPARE, > CPUHP_MM_ZSWP_POOL_PREPARE, > CPUHP_KVM_PPC_BOOK3S_PREPARE, > CPUHP_ZCOMP_PREPARE, > diff --git a/mm/zswap.c b/mm/zswap.c > index 2c349fd88904..37301f1a80a9 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -694,63 +694,31 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) > /********************************* > * per-cpu code > **********************************/ > -static DEFINE_PER_CPU(u8 *, zswap_dstmem); > -/* > - * If users dynamically change the zpool type and compressor at runtime, i.e. > - * zswap is running, zswap can have more than one zpool on one cpu, but they > - * are sharing dtsmem. So we need this mutex to be per-cpu. > - */ > -static DEFINE_PER_CPU(struct mutex *, zswap_mutex); > - > -static int zswap_dstmem_prepare(unsigned int cpu) > -{ > - struct mutex *mutex; > - u8 *dst; > - > - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > - if (!dst) > - return -ENOMEM; > - > - mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); > - if (!mutex) { > - kfree(dst); > - return -ENOMEM; > - } > - > - mutex_init(mutex); > - per_cpu(zswap_dstmem, cpu) = dst; > - per_cpu(zswap_mutex, cpu) = mutex; > - return 0; > -} > - > -static int zswap_dstmem_dead(unsigned int cpu) > -{ > - struct mutex *mutex; > - u8 *dst; > - > - mutex = per_cpu(zswap_mutex, cpu); > - kfree(mutex); > - per_cpu(zswap_mutex, cpu) = NULL; > - > - dst = per_cpu(zswap_dstmem, cpu); > - kfree(dst); > - per_cpu(zswap_dstmem, cpu) = NULL; > - > - return 0; > -} > - > static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > { > struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node); > struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu); > struct crypto_acomp *acomp; > struct acomp_req *req; > + int ret = 0; > + > + acomp_ctx->dstmem = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > + if (!acomp_ctx->dstmem) > + return -ENOMEM; > + > + acomp_ctx->mutex = kmalloc_node(sizeof(struct mutex), GFP_KERNEL, cpu_to_node(cpu)); > + if (!acomp_ctx->mutex) { > + ret = -ENOMEM; > + goto mutex_fail; > + } > + mutex_init(acomp_ctx->mutex); > > acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu)); > if (IS_ERR(acomp)) { > pr_err("could not alloc crypto acomp %s : %ld\n", > pool->tfm_name, PTR_ERR(acomp)); > - return PTR_ERR(acomp); > + ret = PTR_ERR(acomp); > + goto acomp_fail; > } > acomp_ctx->acomp = acomp; > > @@ -758,8 +726,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > if (!req) { > pr_err("could not alloc crypto acomp_request %s\n", > pool->tfm_name); > - crypto_free_acomp(acomp_ctx->acomp); > - return -ENOMEM; > + ret = -ENOMEM; > + goto req_fail; > } > acomp_ctx->req = req; > > @@ -772,10 +740,15 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, > crypto_req_done, &acomp_ctx->wait); > > - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); > - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); > - > return 0; > +req_fail: > + crypto_free_acomp(acomp_ctx->acomp); > +acomp_fail: > + kfree(acomp_ctx->mutex); > +mutex_fail: > + kfree(acomp_ctx->dstmem); > + > + return ret; > } > > static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > @@ -788,6 +761,8 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > acomp_request_free(acomp_ctx->req); > if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) > crypto_free_acomp(acomp_ctx->acomp); > + kfree(acomp_ctx->mutex); > + kfree(acomp_ctx->dstmem); > } > > return 0; > @@ -1901,13 +1876,6 @@ static int zswap_setup(void) > goto cache_fail; > } > > - ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", > - zswap_dstmem_prepare, zswap_dstmem_dead); > - if (ret) { > - pr_err("dstmem alloc failed\n"); > - goto dstmem_fail; > - } > - > ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, > "mm/zswap_pool:prepare", > zswap_cpu_comp_prepare, > @@ -1939,8 +1907,6 @@ static int zswap_setup(void) > if (pool) > zswap_pool_destroy(pool); > hp_fail: > - cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); > -dstmem_fail: > kmem_cache_destroy(zswap_entry_cache); > cache_fail: > /* if built-in, we aren't unloaded on failure; don't allow use */ > -- > 2.20.1 >
Hi Chengming, The patch looks good to me. Acked-by: Chris Li <chrisl@kernel.org> (Google) On Wed, Dec 20, 2023 at 4:21 AM Chengming Zhou <zhouchengming@bytedance.com> wrote: > > On 2023/12/20 05:39, Yosry Ahmed wrote: > > On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote: > >> > >> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: > >>> > >>> Hi Chengming and Yosry, > >>> > >>> On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou > >>> <zhouchengming@bytedance.com> wrote: > >>>> > >>>> Since the introduce of reusing the dstmem in the load path, it seems > >>>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex > >>>> now for purposes other than what the naming suggests. > >>>> > >>>> Yosry suggested removing these two fields from acomp_ctx, and directly > >>>> using zswap_dstmem and zswap_mutex in both the load and store paths, > >>>> rename them, and add proper comments above their definitions that they > >>>> are for generic percpu buffering on the load and store paths. > >>>> > >>>> So this patch remove dstmem and mutex from acomp_ctx, and rename the > >>>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on > >>>> the load and store paths. > >>> > >>> Sorry joining this discussion late. > >>> > >>> I get the rename of "dstmem" to "buffer". Because the buffer is used > >>> for both load and store as well. What I don't get is that, why do we > >>> move it out of the acomp_ctx struct. Now we have 3 per cpu entry: > >>> buffer, mutex and acomp_ctx. I think we should do the reverse, fold > >>> this three per cpu entry into one struct the acomp_ctx. Each per_cpu > >>> load() has a sequence of dance around the cpu id and disable preempt > >>> etc, while each of the struct member load is just a plan memory load. > >>> It seems to me it would be more optimal to combine this three per cpu > >>> entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. > >> > >> I agree with Chris. From a practicality POV, what Chris says here > >> makes sense. From a semantic POV, this buffer is only used in > >> (de)compression contexts - be it in store, load, or writeback - so it > >> belonging to the orignal struct still makes sense to me. Why separate > >> it out, without any benefits. Just rename the old field buffer or > >> zswap_buffer and call it a day? It will be a smaller patch too! > >> > > > > My main concern is that the struct name is specific for the crypto > > acomp stuff, but that buffer and mutex are not. > > How about we keep it in the struct, but refactor the struct as follows: > > > > struct zswap_ctx { > > struct { > > struct crypto_acomp *acomp; > > struct acomp_req *req; > > struct crypto_wait wait; > > } acomp_ctx; > > u8 *dstmem; > > struct mutex *mutex; > > }; > > > > , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx? > > I think there are two viewpoints here, both works ok to me. > > The first is from ownship or lifetime, these percpu mutex and dstmem > are shared between all pools, so no one pool own the mutex and dstmem, > nor does the percpu acomp_ctx in each pool. > > The second is from usage, these percpu mutex and dstmem are used by > the percpu acomp_ctx in each pool, so it's easy to use to put pointers > to them in the percpu acomp_ctx. > > Actually I think it's simpler to let the percpu acomp_ctx has its own > mutex and dstmem, which in fact are the necessary parts when it use > the acomp interfaces. Agree, that is why I prefer to keep the struct together. I am fine with what Yosry suggested and the anonymous struct, just consider it is not critically necessary. > > This way, we could delete the percpu mutex and dstmem, and its hotplugs, That is the real value of this patch. Thanks for doing that. > and not shared anymore between all pools. Maybe we would have many pools > at the same time in the future, like different compression algorithm or > different zpool for different memcg workloads. Who knows? :-) As long as the zswap is not re-enterable, e.g. never have the nested page fault that causes zswap_load within another zswap_load, I think we are fine having more than one pool share the buffer. In fact, if we trigger the nested zswap load, I expect the kernel will dead lock on the nested mutex acquire because the mutex is already taken. We will know about kernel panic rather than selient memory corruption. > > So how about this patch below? Just RFC. > > Subject: [PATCH] mm/zswap: make each acomp_ctx has its own mutex and dstmem Thank you for doing that, you can consider submitting it as the real patch instead of the RFC. I see real value in this patch removing some per CPU fields. > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > include/linux/cpuhotplug.h | 1 - > mm/zswap.c | 86 ++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 61 deletions(-) > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > index efc0c0b07efb..c3e06e21766a 100644 > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -124,7 +124,6 @@ enum cpuhp_state { > CPUHP_ARM_BL_PREPARE, > CPUHP_TRACE_RB_PREPARE, > CPUHP_MM_ZS_PREPARE, > - CPUHP_MM_ZSWP_MEM_PREPARE, > CPUHP_MM_ZSWP_POOL_PREPARE, > CPUHP_KVM_PPC_BOOK3S_PREPARE, > CPUHP_ZCOMP_PREPARE, > diff --git a/mm/zswap.c b/mm/zswap.c > index 2c349fd88904..37301f1a80a9 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -694,63 +694,31 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) > /********************************* > * per-cpu code > **********************************/ > -static DEFINE_PER_CPU(u8 *, zswap_dstmem); > -/* > - * If users dynamically change the zpool type and compressor at runtime, i.e. > - * zswap is running, zswap can have more than one zpool on one cpu, but they > - * are sharing dtsmem. So we need this mutex to be per-cpu. > - */ > -static DEFINE_PER_CPU(struct mutex *, zswap_mutex); > - > -static int zswap_dstmem_prepare(unsigned int cpu) Nice! Chris > -{ > - struct mutex *mutex; > - u8 *dst; > - > - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > - if (!dst) > - return -ENOMEM; > - > - mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); > - if (!mutex) { > - kfree(dst); > - return -ENOMEM; > - } > - > - mutex_init(mutex); > - per_cpu(zswap_dstmem, cpu) = dst; > - per_cpu(zswap_mutex, cpu) = mutex; > - return 0; > -} > - > -static int zswap_dstmem_dead(unsigned int cpu) > -{ > - struct mutex *mutex; > - u8 *dst; > - > - mutex = per_cpu(zswap_mutex, cpu); > - kfree(mutex); > - per_cpu(zswap_mutex, cpu) = NULL; > - > - dst = per_cpu(zswap_dstmem, cpu); > - kfree(dst); > - per_cpu(zswap_dstmem, cpu) = NULL; > - > - return 0; > -} > - > static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > { > struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node); > struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu); > struct crypto_acomp *acomp; > struct acomp_req *req; > + int ret = 0; > + > + acomp_ctx->dstmem = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); > + if (!acomp_ctx->dstmem) > + return -ENOMEM; > + > + acomp_ctx->mutex = kmalloc_node(sizeof(struct mutex), GFP_KERNEL, cpu_to_node(cpu)); > + if (!acomp_ctx->mutex) { > + ret = -ENOMEM; > + goto mutex_fail; > + } > + mutex_init(acomp_ctx->mutex); > > acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu)); > if (IS_ERR(acomp)) { > pr_err("could not alloc crypto acomp %s : %ld\n", > pool->tfm_name, PTR_ERR(acomp)); > - return PTR_ERR(acomp); > + ret = PTR_ERR(acomp); > + goto acomp_fail; > } > acomp_ctx->acomp = acomp; > > @@ -758,8 +726,8 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > if (!req) { > pr_err("could not alloc crypto acomp_request %s\n", > pool->tfm_name); > - crypto_free_acomp(acomp_ctx->acomp); > - return -ENOMEM; > + ret = -ENOMEM; > + goto req_fail; > } > acomp_ctx->req = req; > > @@ -772,10 +740,15 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) > acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, > crypto_req_done, &acomp_ctx->wait); > > - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); > - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); > - > return 0; > +req_fail: > + crypto_free_acomp(acomp_ctx->acomp); > +acomp_fail: > + kfree(acomp_ctx->mutex); > +mutex_fail: > + kfree(acomp_ctx->dstmem); > + > + return ret; > } > > static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > @@ -788,6 +761,8 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > acomp_request_free(acomp_ctx->req); > if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) > crypto_free_acomp(acomp_ctx->acomp); > + kfree(acomp_ctx->mutex); > + kfree(acomp_ctx->dstmem); > } > > return 0; > @@ -1901,13 +1876,6 @@ static int zswap_setup(void) > goto cache_fail; > } > > - ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", > - zswap_dstmem_prepare, zswap_dstmem_dead); > - if (ret) { > - pr_err("dstmem alloc failed\n"); > - goto dstmem_fail; > - } > - > ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, > "mm/zswap_pool:prepare", > zswap_cpu_comp_prepare, > @@ -1939,8 +1907,6 @@ static int zswap_setup(void) > if (pool) > zswap_pool_destroy(pool); > hp_fail: > - cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); > -dstmem_fail: > kmem_cache_destroy(zswap_entry_cache); > cache_fail: > /* if built-in, we aren't unloaded on failure; don't allow use */ > -- > 2.20.1 > >
On 2023/12/23 01:37, Chris Li wrote: > Hi Chengming, > > The patch looks good to me. > > Acked-by: Chris Li <chrisl@kernel.org> (Google) > Thanks. [...] >> >> I think there are two viewpoints here, both works ok to me. >> >> The first is from ownship or lifetime, these percpu mutex and dstmem >> are shared between all pools, so no one pool own the mutex and dstmem, >> nor does the percpu acomp_ctx in each pool. >> >> The second is from usage, these percpu mutex and dstmem are used by >> the percpu acomp_ctx in each pool, so it's easy to use to put pointers >> to them in the percpu acomp_ctx. >> >> Actually I think it's simpler to let the percpu acomp_ctx has its own >> mutex and dstmem, which in fact are the necessary parts when it use >> the acomp interfaces. > > Agree, that is why I prefer to keep the struct together. I am fine > with what Yosry suggested and the anonymous struct, just consider it > is not critically necessary. > Agree, I have no strong opinion about it, so will just leave it as it is. >> >> This way, we could delete the percpu mutex and dstmem, and its hotplugs, > > That is the real value of this patch. Thanks for doing that. > >> and not shared anymore between all pools. Maybe we would have many pools >> at the same time in the future, like different compression algorithm or >> different zpool for different memcg workloads. Who knows? :-) > > As long as the zswap is not re-enterable, e.g. never have the nested > page fault that causes zswap_load within another zswap_load, I think > we are fine having more than one pool share the buffer. In fact, if we > trigger the nested zswap load, I expect the kernel will dead lock on > the nested mutex acquire because the mutex is already taken. We will > know about kernel panic rather than selient memory corruption. > >> >> So how about this patch below? Just RFC. >> >> Subject: [PATCH] mm/zswap: make each acomp_ctx has its own mutex and dstmem > > Thank you for doing that, you can consider submitting it as the real > patch instead of the RFC. I see real value in this patch removing some > per CPU fields. Ok, will update. Thanks!
On 2023/12/21 08:19, Yosry Ahmed wrote: > On Wed, Dec 20, 2023 at 4:20 AM Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> On 2023/12/20 05:39, Yosry Ahmed wrote: >>> On Tue, Dec 19, 2023 at 10:43 AM Nhat Pham <nphamcs@gmail.com> wrote: >>>> >>>> On Tue, Dec 19, 2023 at 5:29 AM Chris Li <chrisl@kernel.org> wrote: >>>>> >>>>> Hi Chengming and Yosry, >>>>> >>>>> On Mon, Dec 18, 2023 at 3:50 AM Chengming Zhou >>>>> <zhouchengming@bytedance.com> wrote: >>>>>> >>>>>> Since the introduce of reusing the dstmem in the load path, it seems >>>>>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex >>>>>> now for purposes other than what the naming suggests. >>>>>> >>>>>> Yosry suggested removing these two fields from acomp_ctx, and directly >>>>>> using zswap_dstmem and zswap_mutex in both the load and store paths, >>>>>> rename them, and add proper comments above their definitions that they >>>>>> are for generic percpu buffering on the load and store paths. >>>>>> >>>>>> So this patch remove dstmem and mutex from acomp_ctx, and rename the >>>>>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on >>>>>> the load and store paths. >>>>> >>>>> Sorry joining this discussion late. >>>>> >>>>> I get the rename of "dstmem" to "buffer". Because the buffer is used >>>>> for both load and store as well. What I don't get is that, why do we >>>>> move it out of the acomp_ctx struct. Now we have 3 per cpu entry: >>>>> buffer, mutex and acomp_ctx. I think we should do the reverse, fold >>>>> this three per cpu entry into one struct the acomp_ctx. Each per_cpu >>>>> load() has a sequence of dance around the cpu id and disable preempt >>>>> etc, while each of the struct member load is just a plan memory load. >>>>> It seems to me it would be more optimal to combine this three per cpu >>>>> entry into acomp_ctx. Just do the per cpu for the acomp_ctx once. >>>> >>>> I agree with Chris. From a practicality POV, what Chris says here >>>> makes sense. From a semantic POV, this buffer is only used in >>>> (de)compression contexts - be it in store, load, or writeback - so it >>>> belonging to the orignal struct still makes sense to me. Why separate >>>> it out, without any benefits. Just rename the old field buffer or >>>> zswap_buffer and call it a day? It will be a smaller patch too! >>>> >>> >>> My main concern is that the struct name is specific for the crypto >>> acomp stuff, but that buffer and mutex are not. >>> How about we keep it in the struct, but refactor the struct as follows: >>> >>> struct zswap_ctx { >>> struct { >>> struct crypto_acomp *acomp; >>> struct acomp_req *req; >>> struct crypto_wait wait; >>> } acomp_ctx; >>> u8 *dstmem; >>> struct mutex *mutex; >>> }; >>> >>> , and then rename zswap_pool.acomp_ctx to zswap_pool.ctx? >> >> I think there are two viewpoints here, both works ok to me. >> >> The first is from ownship or lifetime, these percpu mutex and dstmem >> are shared between all pools, so no one pool own the mutex and dstmem, >> nor does the percpu acomp_ctx in each pool. >> >> The second is from usage, these percpu mutex and dstmem are used by >> the percpu acomp_ctx in each pool, so it's easy to use to put pointers >> to them in the percpu acomp_ctx. >> >> Actually I think it's simpler to let the percpu acomp_ctx has its own >> mutex and dstmem, which in fact are the necessary parts when it use >> the acomp interfaces. >> >> This way, we could delete the percpu mutex and dstmem, and its hotplugs, >> and not shared anymore between all pools. Maybe we would have many pools >> at the same time in the future, like different compression algorithm or >> different zpool for different memcg workloads. Who knows? :-) >> >> So how about this patch below? Just RFC. > > The general approach looks fine to me, although I still prefer we > reorganize the struct as Chris and I discussed: rename > crypto_acomp_ctx to a generic name, add a (anonymous) struct for the > crypto_acomp stuff, rename dstmem to buffer or so. > > I think we can also make the mutex a static part of the struct, any > advantage to dynamically allocating it? Agree, it seems no much advantage to me, I can change to a static part. As for the restructure, I have no strong opinion about it, maybe it's better for me to leave it as it is. Thanks!
diff --git a/mm/zswap.c b/mm/zswap.c index 2c349fd88904..71bdcd552e5b 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -166,8 +166,6 @@ struct crypto_acomp_ctx { struct crypto_acomp *acomp; struct acomp_req *req; struct crypto_wait wait; - u8 *dstmem; - struct mutex *mutex; }; /* @@ -694,7 +692,7 @@ static void zswap_alloc_shrinker(struct zswap_pool *pool) /********************************* * per-cpu code **********************************/ -static DEFINE_PER_CPU(u8 *, zswap_dstmem); +static DEFINE_PER_CPU(u8 *, zswap_buffer); /* * If users dynamically change the zpool type and compressor at runtime, i.e. * zswap is running, zswap can have more than one zpool on one cpu, but they @@ -702,39 +700,39 @@ static DEFINE_PER_CPU(u8 *, zswap_dstmem); */ static DEFINE_PER_CPU(struct mutex *, zswap_mutex); -static int zswap_dstmem_prepare(unsigned int cpu) +static int zswap_buffer_prepare(unsigned int cpu) { struct mutex *mutex; - u8 *dst; + u8 *buf; - dst = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); - if (!dst) + buf = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu)); + if (!buf) return -ENOMEM; mutex = kmalloc_node(sizeof(*mutex), GFP_KERNEL, cpu_to_node(cpu)); if (!mutex) { - kfree(dst); + kfree(buf); return -ENOMEM; } mutex_init(mutex); - per_cpu(zswap_dstmem, cpu) = dst; + per_cpu(zswap_buffer, cpu) = buf; per_cpu(zswap_mutex, cpu) = mutex; return 0; } -static int zswap_dstmem_dead(unsigned int cpu) +static int zswap_buffer_dead(unsigned int cpu) { struct mutex *mutex; - u8 *dst; + u8 *buf; mutex = per_cpu(zswap_mutex, cpu); kfree(mutex); per_cpu(zswap_mutex, cpu) = NULL; - dst = per_cpu(zswap_dstmem, cpu); - kfree(dst); - per_cpu(zswap_dstmem, cpu) = NULL; + buf = per_cpu(zswap_buffer, cpu); + kfree(buf); + per_cpu(zswap_buffer, cpu) = NULL; return 0; } @@ -772,9 +770,6 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node) acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, crypto_req_done, &acomp_ctx->wait); - acomp_ctx->mutex = per_cpu(zswap_mutex, cpu); - acomp_ctx->dstmem = per_cpu(zswap_dstmem, cpu); - return 0; } @@ -1397,15 +1392,21 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) struct zpool *zpool = zswap_find_zpool(entry); struct scatterlist input, output; struct crypto_acomp_ctx *acomp_ctx; - u8 *src; + u8 *src, *buf; + int cpu; + struct mutex *mutex; - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); - mutex_lock(acomp_ctx->mutex); + cpu = raw_smp_processor_id(); + mutex = per_cpu(zswap_mutex, cpu); + mutex_lock(mutex); + + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); 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; + buf = per_cpu(zswap_buffer, cpu); + memcpy(buf, src, entry->length); + src = buf; zpool_unmap_handle(zpool, entry->handle); } @@ -1415,7 +1416,7 @@ static void __zswap_load(struct zswap_entry *entry, struct page *page) acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, PAGE_SIZE); BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)); BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE); - mutex_unlock(acomp_ctx->mutex); + mutex_unlock(mutex); if (zpool_can_sleep_mapped(zpool)) zpool_unmap_handle(zpool, entry->handle); @@ -1551,6 +1552,8 @@ bool zswap_store(struct folio *folio) u8 *src, *dst; gfp_t gfp; int ret; + int cpu; + struct mutex *mutex; VM_WARN_ON_ONCE(!folio_test_locked(folio)); VM_WARN_ON_ONCE(!folio_test_swapcache(folio)); @@ -1636,11 +1639,13 @@ bool zswap_store(struct folio *folio) } /* compress */ - acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx); + cpu = raw_smp_processor_id(); + mutex = per_cpu(zswap_mutex, cpu); + mutex_lock(mutex); - mutex_lock(acomp_ctx->mutex); + acomp_ctx = per_cpu_ptr(entry->pool->acomp_ctx, cpu); + dst = per_cpu(zswap_buffer, cpu); - dst = acomp_ctx->dstmem; sg_init_table(&input, 1); sg_set_page(&input, page, PAGE_SIZE, 0); @@ -1683,7 +1688,7 @@ bool zswap_store(struct folio *folio) buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO); memcpy(buf, dst, dlen); zpool_unmap_handle(zpool, handle); - mutex_unlock(acomp_ctx->mutex); + mutex_unlock(mutex); /* populate entry */ entry->swpentry = swp_entry(type, offset); @@ -1726,7 +1731,7 @@ bool zswap_store(struct folio *folio) return true; put_dstmem: - mutex_unlock(acomp_ctx->mutex); + mutex_unlock(mutex); put_pool: zswap_pool_put(entry->pool); freepage: @@ -1902,10 +1907,10 @@ static int zswap_setup(void) } ret = cpuhp_setup_state(CPUHP_MM_ZSWP_MEM_PREPARE, "mm/zswap:prepare", - zswap_dstmem_prepare, zswap_dstmem_dead); + zswap_buffer_prepare, zswap_buffer_dead); if (ret) { - pr_err("dstmem alloc failed\n"); - goto dstmem_fail; + pr_err("buffer alloc failed\n"); + goto buffer_fail; } ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE, @@ -1940,7 +1945,7 @@ static int zswap_setup(void) zswap_pool_destroy(pool); hp_fail: cpuhp_remove_state(CPUHP_MM_ZSWP_MEM_PREPARE); -dstmem_fail: +buffer_fail: kmem_cache_destroy(zswap_entry_cache); cache_fail: /* if built-in, we aren't unloaded on failure; don't allow use */
Since the introduce of reusing the dstmem in the load path, it seems confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex now for purposes other than what the naming suggests. Yosry suggested removing these two fields from acomp_ctx, and directly using zswap_dstmem and zswap_mutex in both the load and store paths, rename them, and add proper comments above their definitions that they are for generic percpu buffering on the load and store paths. So this patch remove dstmem and mutex from acomp_ctx, and rename the zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on the load and store paths. Suggested-by: Yosry Ahmed <yosryahmed@google.com> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> --- mm/zswap.c | 69 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 37 insertions(+), 32 deletions(-)