RE: [PATCH 8/8] mm/zswap: Use local lock to protect per-CPU data

[Song Bao Hua]

> On 2020-05-19 21:46:06 [+0000], Song Bao Hua wrote:
> > Hi Luis,
> > In the below patch, in order to use the acomp APIs to leverage the power of
> hardware compressors. I have moved to mutex:
> > https://marc.info/?l=linux-crypto-vger&m=158941285830302&w=2
> > https://marc.info/?l=linux-crypto-vger&m=158941287930311&w=2
> >
> > so once we get some progress on that one, I guess we don't need a special
> patch for RT any more.
> 
> If you convert this way from the current concept then we could drop it from
> the series.
> The second patch shows the following hunk:
> 
> |@@ -1075,11 +1124,20 @@ static int zswap_frontswap_store(unsigned
> type,
> |pgoff_t offset,
> |
> | 	/* compress */
> | 	dst = get_cpu_var(zswap_dstmem);
> |	acomp_ctx = *this_cpu_ptr(entry->pool->acomp_ctx);
> |	put_cpu_var(zswap_dstmem);
> 
> So here you get per-CPU version of `dst' and `acomp_ctx' and then allow
> preemption again.
> 
> |	mutex_lock(&acomp_ctx->mutex);
> |
> |	src = kmap(page);
> |	sg_init_one(&input, src, PAGE_SIZE);
> |	/* zswap_dstmem is of size (PAGE_SIZE * 2). Reflect same in sg_list */
> |	sg_init_one(&output, dst, PAGE_SIZE * 2);
> 
> and here you use `dst' and `acomp_ctx' after the preempt_disable() has been
> dropped so I don't understand why you used get_cpu_var(). It is either
> protected by the mutex and doesn't require get_cpu_var() or it isn't (and
> should have additional protection).

The old code was like:
For each cpu, there is one percpu comp and one percpu pages for compression destination - zswap_dstmem.
For example, on cpu1, once you begin to compress, you hold the percpu comp and percpu destination buffer. Meanwhile, preemption is disabled. So decompression won't be able to work at the same core in parallel. And two compressions won't be able to do at the same core in parallel as well. At the same time, the thread won't be able to migrate to another core. Other cores might can do compression/decompression in parallel

The new code is like:
For each cpu, there is still one percpu acomp-ctx and one percpu pages for compression destination. Here acomp replaces comp, and acomp requires sleep during compressing and decompressing.
For example, on cpu1, once you begin to compress, you hold the percpu acomp-ctx and percpu destination buffer of CPU1, the below code makes sure you get the acomp and dstmem from the same core by disabling preemption with get_cpu_var and put_cpu_var:
dst = get_cpu_var(zswap_dstmem);
acomp_ctx = *this_cpu_ptr(entry->pool->acomp_ctx);
put_cpu_var(zswap_dstmem);

then there are two cases:

1. after getting dst and acomp_ctx of cpu1, you might always work in cpu1, the mutex in per-cpu acomp-ctx will guarantee two compressions won't do at the same core in parallel, and it also makes certain compression and decompression won't do at the same core in parallel. Everything is like before.

2. after getting dst and acomp_ctx of cpu1, you might migrate to cpu2, but even you move to cpu2, you are still holding the mutex of cpu1's acomp-ctx.
If at that time, cpu1 has another request to do compression, it will be blocked by the mutex held by cpu2.
If at that time, cpu1 wants to do decompression, it wil be blocked by the mutex held by cpu2.

Everything is like before. No matter which core you have migrated to, once you hold the mutex of core N, another compression/decompression who wants to hold the mutex of core N will be blocked. So mostly, if you have M cores, you can do M compression/decompression in parallel like before.

My basic idea is keeping the work model unchanged like before.

> 
> |	acomp_request_set_params(acomp_ctx->req, &input, &output,
> PAGE_SIZE, dlen);
> |	ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req),
> &acomp_ctx->wait);
> |	dlen = acomp_ctx->req->dlen;
> |	kunmap(page);
> |
> | 	if (ret) {
> | 		ret = -EINVAL;
> | 		goto put_dstmem;
> |
> 
> Sebastian

Barry