On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
<[email protected]> wrote:
>
> In the !zpool_can_sleep_mapped() case such as zsmalloc, we need to first
> copy the entry->handle memory to a temporary memory, which is allocated
> using kmalloc.
>
> Obviously we can reuse the per-compressor dstmem to avoid allocating
> every time, since it's percpu-compressor and protected in mutex.
>
> Signed-off-by: Chengming Zhou <[email protected]>
> Reviewed-by: Nhat Pham <[email protected]>
> ---
> mm/zswap.c | 29 +++++++++--------------------
> 1 file changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 7ee54a3d8281..edb8b45ed5a1 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1772,9 +1772,9 @@ bool zswap_load(struct folio *folio)
> struct zswap_entry *entry;
> struct scatterlist input, output;
> struct crypto_acomp_ctx *acomp_ctx;
> - u8 *src, *dst, *tmp;
> + unsigned int dlen = PAGE_SIZE;
> + u8 *src, *dst;
> struct zpool *zpool;
> - unsigned int dlen;
> bool ret;
>
> VM_WARN_ON_ONCE(!folio_test_locked(folio));
> @@ -1796,27 +1796,18 @@ bool zswap_load(struct folio *folio)
> goto stats;
> }
>
> - zpool = zswap_find_zpool(entry);
> - if (!zpool_can_sleep_mapped(zpool)) {
> - tmp = kmalloc(entry->length, GFP_KERNEL);
> - if (!tmp) {
> - ret = false;
> - goto freeentry;
> - }
> - }
> -
> /* decompress */
> - dlen = PAGE_SIZE;
> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> + 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(tmp, src, entry->length);
> - src = tmp;
> + memcpy(acomp_ctx->dstmem, src, entry->length);
> + src = acomp_ctx->dstmem;
I don't like that we are now using acomp_ctx->dstmem and
acomp_ctx->mutex now for purposes other than what the naming suggests.
How about 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?
On 2023/12/14 07:24, Yosry Ahmed wrote:
> On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
> <[email protected]> wrote:
>>
>> In the !zpool_can_sleep_mapped() case such as zsmalloc, we need to first
>> copy the entry->handle memory to a temporary memory, which is allocated
>> using kmalloc.
>>
>> Obviously we can reuse the per-compressor dstmem to avoid allocating
>> every time, since it's percpu-compressor and protected in mutex.
>>
>> Signed-off-by: Chengming Zhou <[email protected]>
>> Reviewed-by: Nhat Pham <[email protected]>
>> ---
>> mm/zswap.c | 29 +++++++++--------------------
>> 1 file changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 7ee54a3d8281..edb8b45ed5a1 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -1772,9 +1772,9 @@ bool zswap_load(struct folio *folio)
>> struct zswap_entry *entry;
>> struct scatterlist input, output;
>> struct crypto_acomp_ctx *acomp_ctx;
>> - u8 *src, *dst, *tmp;
>> + unsigned int dlen = PAGE_SIZE;
>> + u8 *src, *dst;
>> struct zpool *zpool;
>> - unsigned int dlen;
>> bool ret;
>>
>> VM_WARN_ON_ONCE(!folio_test_locked(folio));
>> @@ -1796,27 +1796,18 @@ bool zswap_load(struct folio *folio)
>> goto stats;
>> }
>>
>> - zpool = zswap_find_zpool(entry);
>> - if (!zpool_can_sleep_mapped(zpool)) {
>> - tmp = kmalloc(entry->length, GFP_KERNEL);
>> - if (!tmp) {
>> - ret = false;
>> - goto freeentry;
>> - }
>> - }
>> -
>> /* decompress */
>> - dlen = PAGE_SIZE;
>> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
>> + 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(tmp, src, entry->length);
>> - src = tmp;
>> + memcpy(acomp_ctx->dstmem, src, entry->length);
>> + src = acomp_ctx->dstmem;
>
> I don't like that we are now using acomp_ctx->dstmem and
> acomp_ctx->mutex now for purposes other than what the naming suggests.
The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
Change to just "mem"? Or do you have a better name to replace?
>
> How about 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?
Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
and the cpu maybe changing in the middle, so maybe better to keep them.
Thanks!
On Thu, Dec 14, 2023 at 5:29 AM Chengming Zhou
<[email protected]> wrote:
>
> On 2023/12/14 07:24, Yosry Ahmed wrote:
> > On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
> > <[email protected]> wrote:
> >>
> >> In the !zpool_can_sleep_mapped() case such as zsmalloc, we need to first
> >> copy the entry->handle memory to a temporary memory, which is allocated
> >> using kmalloc.
> >>
> >> Obviously we can reuse the per-compressor dstmem to avoid allocating
> >> every time, since it's percpu-compressor and protected in mutex.
> >>
> >> Signed-off-by: Chengming Zhou <[email protected]>
> >> Reviewed-by: Nhat Pham <[email protected]>
> >> ---
> >> mm/zswap.c | 29 +++++++++--------------------
> >> 1 file changed, 9 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 7ee54a3d8281..edb8b45ed5a1 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -1772,9 +1772,9 @@ bool zswap_load(struct folio *folio)
> >> struct zswap_entry *entry;
> >> struct scatterlist input, output;
> >> struct crypto_acomp_ctx *acomp_ctx;
> >> - u8 *src, *dst, *tmp;
> >> + unsigned int dlen = PAGE_SIZE;
> >> + u8 *src, *dst;
> >> struct zpool *zpool;
> >> - unsigned int dlen;
> >> bool ret;
> >>
> >> VM_WARN_ON_ONCE(!folio_test_locked(folio));
> >> @@ -1796,27 +1796,18 @@ bool zswap_load(struct folio *folio)
> >> goto stats;
> >> }
> >>
> >> - zpool = zswap_find_zpool(entry);
> >> - if (!zpool_can_sleep_mapped(zpool)) {
> >> - tmp = kmalloc(entry->length, GFP_KERNEL);
> >> - if (!tmp) {
> >> - ret = false;
> >> - goto freeentry;
> >> - }
> >> - }
> >> -
> >> /* decompress */
> >> - dlen = PAGE_SIZE;
> >> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> >> + 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(tmp, src, entry->length);
> >> - src = tmp;
> >> + memcpy(acomp_ctx->dstmem, src, entry->length);
> >> + src = acomp_ctx->dstmem;
> >
> > I don't like that we are now using acomp_ctx->dstmem and
> > acomp_ctx->mutex now for purposes other than what the naming suggests.
>
> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
> Change to just "mem"? Or do you have a better name to replace?
>
> >
> > How about 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?
>
> Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
> and the cpu maybe changing in the middle, so maybe better to keep them.
I don't mean to remove completely. Keep them as (for example)
zswap_mem and zswap_mutex global percpu variables, and not have
pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
today, we directly use the global zswap_mem (same for the mutex).
This makes it clear that the buffers are not owned or exclusively used
by the acomp_ctx. WDYT?
On 2023/12/14 21:32, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 5:29 AM Chengming Zhou
> <[email protected]> wrote:
>>
>> On 2023/12/14 07:24, Yosry Ahmed wrote:
>>> On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
>>> <[email protected]> wrote:
>>>>
>>>> In the !zpool_can_sleep_mapped() case such as zsmalloc, we need to first
>>>> copy the entry->handle memory to a temporary memory, which is allocated
>>>> using kmalloc.
>>>>
>>>> Obviously we can reuse the per-compressor dstmem to avoid allocating
>>>> every time, since it's percpu-compressor and protected in mutex.
>>>>
>>>> Signed-off-by: Chengming Zhou <[email protected]>
>>>> Reviewed-by: Nhat Pham <[email protected]>
>>>> ---
>>>> mm/zswap.c | 29 +++++++++--------------------
>>>> 1 file changed, 9 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/mm/zswap.c b/mm/zswap.c
>>>> index 7ee54a3d8281..edb8b45ed5a1 100644
>>>> --- a/mm/zswap.c
>>>> +++ b/mm/zswap.c
>>>> @@ -1772,9 +1772,9 @@ bool zswap_load(struct folio *folio)
>>>> struct zswap_entry *entry;
>>>> struct scatterlist input, output;
>>>> struct crypto_acomp_ctx *acomp_ctx;
>>>> - u8 *src, *dst, *tmp;
>>>> + unsigned int dlen = PAGE_SIZE;
>>>> + u8 *src, *dst;
>>>> struct zpool *zpool;
>>>> - unsigned int dlen;
>>>> bool ret;
>>>>
>>>> VM_WARN_ON_ONCE(!folio_test_locked(folio));
>>>> @@ -1796,27 +1796,18 @@ bool zswap_load(struct folio *folio)
>>>> goto stats;
>>>> }
>>>>
>>>> - zpool = zswap_find_zpool(entry);
>>>> - if (!zpool_can_sleep_mapped(zpool)) {
>>>> - tmp = kmalloc(entry->length, GFP_KERNEL);
>>>> - if (!tmp) {
>>>> - ret = false;
>>>> - goto freeentry;
>>>> - }
>>>> - }
>>>> -
>>>> /* decompress */
>>>> - dlen = PAGE_SIZE;
>>>> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
>>>> + 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(tmp, src, entry->length);
>>>> - src = tmp;
>>>> + memcpy(acomp_ctx->dstmem, src, entry->length);
>>>> + src = acomp_ctx->dstmem;
>>>
>>> I don't like that we are now using acomp_ctx->dstmem and
>>> acomp_ctx->mutex now for purposes other than what the naming suggests.
>>
>> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
>> Change to just "mem"? Or do you have a better name to replace?
>>
>>>
>>> How about 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?
>>
>> Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
>> and the cpu maybe changing in the middle, so maybe better to keep them.
>
> I don't mean to remove completely. Keep them as (for example)
> zswap_mem and zswap_mutex global percpu variables, and not have
> pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
> today, we directly use the global zswap_mem (same for the mutex).
>
> This makes it clear that the buffers are not owned or exclusively used
> by the acomp_ctx. WDYT?
Does this look good to you?
```
int cpu = raw_smp_processor_id();
mutex = per_cpu(zswap_mutex, cpu);
mutex_lock(mutex);
dstmem = per_cpu(zswap_dstmem, cpu);
acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);
/* compress or decompress */
```
Another way I just think of is to make acomp_ctx own its lock and buffer,
and we could delete these percpu zswap_mutex and zswap_dstmem instead.
Thanks.
On Thu, Dec 14, 2023 at 6:42 AM Chengming Zhou
<[email protected]> wrote:
[..]
> >>>> -
> >>>> /* decompress */
> >>>> - dlen = PAGE_SIZE;
> >>>> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> >>>> + 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(tmp, src, entry->length);
> >>>> - src = tmp;
> >>>> + memcpy(acomp_ctx->dstmem, src, entry->length);
> >>>> + src = acomp_ctx->dstmem;
> >>>
> >>> I don't like that we are now using acomp_ctx->dstmem and
> >>> acomp_ctx->mutex now for purposes other than what the naming suggests.
> >>
> >> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
> >> Change to just "mem"? Or do you have a better name to replace?
> >>
> >>>
> >>> How about 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?
> >>
> >> Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
> >> and the cpu maybe changing in the middle, so maybe better to keep them.
> >
> > I don't mean to remove completely. Keep them as (for example)
> > zswap_mem and zswap_mutex global percpu variables, and not have
> > pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
> > today, we directly use the global zswap_mem (same for the mutex).
> >
> > This makes it clear that the buffers are not owned or exclusively used
> > by the acomp_ctx. WDYT?
>
> Does this look good to you?
>
> ```
> int cpu = raw_smp_processor_id();
>
> mutex = per_cpu(zswap_mutex, cpu);
> mutex_lock(mutex);
>
> dstmem = per_cpu(zswap_dstmem, cpu);
Renaming to zswap_buffer or zswap_mem would be better I think, but
yeah what I had in mind is having zswap_mutex and
zswap_[dstmem/mem/buffer] be generic percpu buffers that are used by
store and load paths for different purposes, not directly linked to
acomp_ctx.
> acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);
>
> /* compress or decompress */
> ```
>
> Another way I just think of is to make acomp_ctx own its lock and buffer,
> and we could delete these percpu zswap_mutex and zswap_dstmem instead.
You mean have two separate set of percpu buffers for zswap load &
stores paths? This is probably unnecessary.
On 2023/12/15 02:24, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 6:42 AM Chengming Zhou
> <[email protected]> wrote:
> [..]
>>>>>> -
>>>>>> /* decompress */
>>>>>> - dlen = PAGE_SIZE;
>>>>>> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
>>>>>> + 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(tmp, src, entry->length);
>>>>>> - src = tmp;
>>>>>> + memcpy(acomp_ctx->dstmem, src, entry->length);
>>>>>> + src = acomp_ctx->dstmem;
>>>>>
>>>>> I don't like that we are now using acomp_ctx->dstmem and
>>>>> acomp_ctx->mutex now for purposes other than what the naming suggests.
>>>>
>>>> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
>>>> Change to just "mem"? Or do you have a better name to replace?
>>>>
>>>>>
>>>>> How about 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?
>>>>
>>>> Yes, they are percpu memory and lock, but they are used by per acomp_ctx,
>>>> and the cpu maybe changing in the middle, so maybe better to keep them.
>>>
>>> I don't mean to remove completely. Keep them as (for example)
>>> zswap_mem and zswap_mutex global percpu variables, and not have
>>> pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
>>> today, we directly use the global zswap_mem (same for the mutex).
>>>
>>> This makes it clear that the buffers are not owned or exclusively used
>>> by the acomp_ctx. WDYT?
>>
>> Does this look good to you?
>>
>> ```
>> int cpu = raw_smp_processor_id();
>>
>> mutex = per_cpu(zswap_mutex, cpu);
>> mutex_lock(mutex);
>>
>> dstmem = per_cpu(zswap_dstmem, cpu);
>
> Renaming to zswap_buffer or zswap_mem would be better I think, but
> yeah what I had in mind is having zswap_mutex and
> zswap_[dstmem/mem/buffer] be generic percpu buffers that are used by
> store and load paths for different purposes, not directly linked to
> acomp_ctx.
>
Ok, I'll append a patch to do the refactor & cleanup: remove mutex
and dstmem from acomp_ctx, and rename to zswap_buffer, then directly
use them on the load/store paths.
>> acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);
>>
>> /* compress or decompress */
>> ```
>>
>> Another way I just think of is to make acomp_ctx own its lock and buffer,
>> and we could delete these percpu zswap_mutex and zswap_dstmem instead.
>
> You mean have two separate set of percpu buffers for zswap load &
> stores paths? This is probably unnecessary.
Alright. Thanks.