crypto: scompress: remove memcpy if sg_nents is 1 and pages are lowmem
PATCH v6 1/1 crypto: scompress: remove memcpy if sg_nents is 1 and pages are lowmem
From: Barry Song <[email protected]>
while sg_nents is 1, which is always true for the current kernel as the only user - zswap is this case, we might have a chance to remove memcpy, thus improve the performance.
Though sg_nents is 1, its buffer might cross two pages. If those pages are highmem, we have no cheap way to map them to contiguous virtual address because kmap doesn't support more than one page (kmap single higmem page could be still expensive for tlb) and vmap is expensive.
So we also test and enure page is not highmem in order to safely use page_to_virt before removing the memcpy. The good news is that in the most majority of cases, we are lowmem, and we are always lowmem in those modern and popular hardware.
---
-v6:
* separate it from zswap & crypto series[1] according to Herbert;
* make sure the code is still safe when buffers cross two pages to address Herbert's comment
[1] https://lore.kernel.org/linux-mm/[email protected]/
crypto/scompress.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
crypto/scompress.c +24 -7
@@ -117,6 +117,7 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)struct crypto_scomp *scomp = *tfm_ctx;void **ctx =acomp_request_ctx (req);struct scomp_scratch *scratch; +void *src, *dst;unsigned int dlen;int ret;
@@ -134,13 +135,25 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) scratch =raw_cpu_ptr (&scomp_scratch);spin_lock (&scratch->lock); -scatterwalk_map_and_copy (scratch->src, req->src,0 , req->slen,0 ); +if (sg_nents (req->src) ==1 && !PageHighMem (sg_page (req->src))) { + src =page_to_virt (sg_page (req->src)) + req->src->offset; + }else { +scatterwalk_map_and_copy (scratch->src, req->src,0 , + req->slen,0 ); + src = scratch->src; + } + +if (req->dst &&sg_nents (req->dst) ==1 && !PageHighMem (sg_page (req->dst))) + dst =page_to_virt (sg_page (req->dst)) + req->dst->offset; +else + dst = scratch->dst; +if (dir) - ret =crypto_scomp_compress (scomp, scratch->src, req->slen, - scratch->dst, &req->dlen, *ctx); + ret =crypto_scomp_compress (scomp, src, req->slen, + dst, &req->dlen, *ctx);else - ret =crypto_scomp_decompress (scomp, scratch->src, req->slen, - scratch->dst, &req->dlen, *ctx); + ret =crypto_scomp_decompress (scomp, src, req->slen, + dst, &req->dlen, *ctx);if (!ret) {if (!req->dst) { req->dst =sgl_alloc (req->dlen,GFP_ATOMIC ,NULL );
@@ -152,8 +165,12 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) ret = -ENOSPC ; goto out; } -scatterwalk_map_and_copy (scratch->dst, req->dst,0 , req->dlen, -1 ); +if (dst == scratch->dst) { +scatterwalk_map_and_copy (scratch->dst, req->dst,0 , + req->dlen,1 ); + }else { +flush_dcache_page (sg_page (req->dst)); + } } out:spin_unlock (&scratch->lock);
Signature · 1 line
2.34.1
Herbert Xu
maintainercrypto/scompress.c static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) · line 139
Quoted 4 lines · Barry Song
crypto/scompress.c +2
-1-scatterwalk_map_and_copy (scratch->src, req->src,0 , req->slen,0 ); +if (sg_nents (req->src) ==1 && !PageHighMem (sg_page (req->src))) { + src =page_to_virt (sg_page (req->src)) + req->src->offset;
Incidentally this made me look at other uses of PageHighMem in the kernel.
The one in copy_page_from_iter_atomic looks buggy because it assumes that the kmap never maps a page if PageHighMem is false, which is not the case for CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP.
crypto/scompress.c static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) · line 172
Quoted 11 lines
crypto/scompress.c +5
-2@@ -152,8 +165,12 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) ret = -ENOSPC ; goto out; } -scatterwalk_map_and_copy (scratch->dst, req->dst,0 , req->dlen, -1 ); +if (dst == scratch->dst) { +scatterwalk_map_and_copy (scratch->dst, req->dst,0 , + req->dlen,1 ); + }else { +flush_dcache_page (sg_page (req->dst));
I think this is still wrong for the > PAGE_SIZE case. The existing code flushes each page sequentially but the new code only flushes the first page.
Thanks,
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Quoted 7 lines · Herbert Xu
Review
Quoted 4 lines · Barry Song
(quoted diff) +2
-1- scatterwalk_map_and_copy(scratch->src, req->src, 0, req->slen, 0); + if (sg_nents(req->src) == 1 && !PageHighMem(sg_page(req->src))) { + src = page_to_virt(sg_page(req->src)) + req->src->offset;
Quoted 6 lines
Incidentally this made me look at other uses of PageHighMem in the kernel.
The one in
copy_page_from_iter_atomiclooks buggy because it assumes that the kmap never maps a page if PageHighMem is false, which is not the case forCONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP.
You are right. This needs to be fixed.
Quoted 16 lines
Review static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) · line 172
Quoted 12 lines
(quoted diff) +5
-2@@ -152,8 +165,12 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir) ret = -ENOSPC; goto out; } - scatterwalk_map_and_copy(scratch->dst, req->dst, 0, req->dlen, - 1); + if (dst == scratch->dst) { + scatterwalk_map_and_copy(scratch->dst, req->dst, 0, + req->dlen, 1); + } else { + flush_dcache_page(sg_page(req->dst));I think this is still wrong for the >
PAGE_SIZEcase. The existing code flushes each page sequentially but the new code only flushes the first page.
right, can it be fixed like the below?
crypto/scompress.c +0 -0
@@ -169,7 +169,11 @@ static int scomp_acomp_comp_decomp(struct
acomp_req *req, int dir)
scatterwalk_map_and_copy(scratch->dst, req->dst, 0,
req->dlen, 1);
} else {- flush_dcache_page(sg_page(req->dst));
+ int nr_pages = DIV_ROUND_UP(req->dst->offset +req->dlen, PAGE_SIZE);
+ int i;
+
+ for (i = 0; i < nr_pages; i++)
+ flush_dcache_page(sg_page(req->dst) + i);
}
}out:
Quoted 7 lines
Thanks,
--
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Thanks
Barry