| Message ID | 20240222081135.173040-1-21cnbao@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | zswap: remove the memcpy if acomp is not sleepable | expand |
Hi Andrew, On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote: > > From: Barry Song <v-songbaohua@oppo.com> > > In zswap, if we use zsmalloc, we cannot sleep while we map the > compressed memory, so we copy it to a temporary buffer. By > knowing the alg won't sleep can help zswap to avoid the > memcpy. > Thus we introduce an API in crypto to expose if acomp is async, > and zswap can use it to decide if it can remove copying to the > tmp buffer. > > -v6: > * add acked-by of Herbert, Thanks! > * remove patch 3/3 from the series, as that one will go > through crypto Can you please pull this into mm-tree? This used to have 3 patches. 3/3 was separated according to Herbert's requirements and has been in a crypto tree. crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca Two drivers fixes(patch 1 needs) have also been in crypto tree: crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385 crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35 So it should be quite safe to pull this series into mm-tree now. > > Barry Song (2): > crypto: introduce: acomp_is_async to expose if comp drivers might > sleep > mm/zswap: remove the memcpy if acomp is not sleepable > > include/crypto/acompress.h | 6 ++++++ > mm/zswap.c | 6 ++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > -- > 2.34.1 Thanks Barry
On Thu, Feb 22, 2024 at 09:11:33PM +1300, Barry Song wrote: > From: Barry Song <v-songbaohua@oppo.com> > > In zswap, if we use zsmalloc, we cannot sleep while we map the > compressed memory, so we copy it to a temporary buffer. By > knowing the alg won't sleep can help zswap to avoid the > memcpy. > Thus we introduce an API in crypto to expose if acomp is async, > and zswap can use it to decide if it can remove copying to the > tmp buffer. > > -v6: > * add acked-by of Herbert, Thanks! > * remove patch 3/3 from the series, as that one will go > through crypto > > Barry Song (2): > crypto: introduce: acomp_is_async to expose if comp drivers might > sleep > mm/zswap: remove the memcpy if acomp is not sleepable > > include/crypto/acompress.h | 6 ++++++ > mm/zswap.c | 6 ++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) Acked-by: Johannes Weiner <hannes@cmpxchg.org> Looks good to me. One small question: why cache is_sleepable in zswap instead of checking acomp_is_async() directly? It doesn't look expensive.
On Fri, 8 Mar 2024 19:57:38 +0800 Barry Song <21cnbao@gmail.com> wrote: > Hi Andrew, > > On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote: > > > > From: Barry Song <v-songbaohua@oppo.com> > > > > In zswap, if we use zsmalloc, we cannot sleep while we map the > > compressed memory, so we copy it to a temporary buffer. By > > knowing the alg won't sleep can help zswap to avoid the > > memcpy. > > Thus we introduce an API in crypto to expose if acomp is async, > > and zswap can use it to decide if it can remove copying to the > > tmp buffer. > > > > -v6: > > * add acked-by of Herbert, Thanks! > > * remove patch 3/3 from the series, as that one will go > > through crypto > > Can you please pull this into mm-tree? This used to have 3 patches. > > 3/3 was separated according to Herbert's requirements and has > been in a crypto tree. > crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca > > Two drivers fixes(patch 1 needs) have also been in crypto tree: > crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385 > > crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35 > > So it should be quite safe to pull this series into mm-tree now. But this zswap chage requires the presence of the other patches, yes? So the mm.git tree alone will be buggy? And if mm.git merges ahead of the other trees, there will be a window where mainline will be buggy? If so, I think it wuold be better to merge the zswap patch in the next merge window.
On Sat, Mar 9, 2024 at 11:23 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 8 Mar 2024 19:57:38 +0800 Barry Song <21cnbao@gmail.com> wrote: > > > Hi Andrew, > > > > On Thu, Feb 22, 2024 at 4:11 PM Barry Song <21cnbao@gmail.com> wrote: > > > > > > From: Barry Song <v-songbaohua@oppo.com> > > > > > > In zswap, if we use zsmalloc, we cannot sleep while we map the > > > compressed memory, so we copy it to a temporary buffer. By > > > knowing the alg won't sleep can help zswap to avoid the > > > memcpy. > > > Thus we introduce an API in crypto to expose if acomp is async, > > > and zswap can use it to decide if it can remove copying to the > > > tmp buffer. > > > > > > -v6: > > > * add acked-by of Herbert, Thanks! > > > * remove patch 3/3 from the series, as that one will go > > > through crypto > > > > Can you please pull this into mm-tree? This used to have 3 patches. > > > > 3/3 was separated according to Herbert's requirements and has > > been in a crypto tree. > > crypto: scomp - remove memcpy if sg_nents is 1 and pages are lowmem > > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=77292bb8ca > > > > Two drivers fixes(patch 1 needs) have also been in crypto tree: > > crypto: hisilicon/zip - fix the missing CRYPTO_ALG_ASYNC in cra_flags > > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=db8ac88385 > > > > crypto: iaa - fix the missing CRYPTO_ALG_ASYNC in cra_flags > > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=30dd94dba35 > > > > So it should be quite safe to pull this series into mm-tree now. > > But this zswap chage requires the presence of the other patches, yes? As far as I understand, we rely on two driver fixes because those drivers didn't set the correct cra_flags needed by our patch1. Without those fixes implemented, two platforms might encounter issues: Intel with IAA (Intel Analytics Accelerator) and Hisilicon with ZIP. Other platforms should be unaffected. The two driver fixes have been merged into the crypto tree. > > So the mm.git tree alone will be buggy? And if mm.git merges ahead of > the other trees, there will be a window where mainline will be buggy? Before 6.9-rc1, there might be issues if mm enters Linus' tree before Herbert's crypto tree. However, by 6.9-rc1, everything should be fine. > > If so, I think it wuold be better to merge the zswap patch in the next > merge window. > Okay, I understand. Since this patch improves zswap's performance, I wanted it to be integrated sooner to contribute. However, I'm perfectly willing to respect your concerns and adhere to the community's best practices. So please handle it in the best way you think :-) Thanks Barry
On Sat, 9 Mar 2024 11:58:39 +0800 Barry Song <21cnbao@gmail.com> wrote: > > > > > > So it should be quite safe to pull this series into mm-tree now. > > > > But this zswap chage requires the presence of the other patches, yes? > > As far as I understand, we rely on two driver fixes because those drivers didn't > set the correct cra_flags needed by our patch1. Without those fixes implemented, > two platforms might encounter issues: Intel with IAA (Intel Analytics > Accelerator) > and Hisilicon with ZIP. Other platforms should be unaffected. > > The two driver fixes have been merged into the crypto tree. > > > > > So the mm.git tree alone will be buggy? And if mm.git merges ahead of > > the other trees, there will be a window where mainline will be buggy? > > Before 6.9-rc1, there might be issues if mm enters Linus' tree before Herbert's > crypto tree. However, by 6.9-rc1, everything should be fine. > > > > > If so, I think it wuold be better to merge the zswap patch in the next > > merge window. > > > > Okay, I understand. Since this patch improves zswap's performance, I wanted > it to be integrated sooner to contribute. However, I'm perfectly willing to > respect your concerns and adhere to the community's best practices. > OK. I very much doubt if anyone is running those drivers on mm.git, so adding it now isn't likely to hurt. So I'll merge it now and shall aim to get it upstream very late in the next merge window.
On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > Okay, I understand. Since this patch improves zswap's performance, I wanted > > it to be integrated sooner to contribute. However, I'm perfectly willing to > > respect your concerns and adhere to the community's best practices. > > > > OK. I very much doubt if anyone is running those drivers on mm.git, so > adding it now isn't likely to hurt. > > So I'll merge it now and shall aim to get it upstream very late in the > next merge window. Nope. mm.git won't build without acomp_is_async(). We can merge the zswap patch via the crypto tree. Acked-by: me. Or please just resend the zswap change after 6.9-rc1 is released.
On Sat, Mar 9, 2024 at 12:42 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > > > Okay, I understand. Since this patch improves zswap's performance, I wanted > > > it to be integrated sooner to contribute. However, I'm perfectly willing to > > > respect your concerns and adhere to the community's best practices. > > > > > > > OK. I very much doubt if anyone is running those drivers on mm.git, so > > adding it now isn't likely to hurt. > > > > So I'll merge it now and shall aim to get it upstream very late in the > > next merge window. > > Nope. mm.git won't build without acomp_is_async(). > > We can merge the zswap patch via the crypto tree. Acked-by: me. Herbert Acked the acomp_is_async() patch in v5 instead of picking it up into crypto: https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/ > > Or please just resend the zswap change after 6.9-rc1 is released. > Thanks Barry Thanks Barry
On Sat, Mar 9, 2024 at 12:56 PM Barry Song <21cnbao@gmail.com> wrote: > > On Sat, Mar 9, 2024 at 12:42 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Fri, 8 Mar 2024 20:36:41 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > Okay, I understand. Since this patch improves zswap's performance, I wanted > > > > it to be integrated sooner to contribute. However, I'm perfectly willing to > > > > respect your concerns and adhere to the community's best practices. > > > > > > > > > > OK. I very much doubt if anyone is running those drivers on mm.git, so > > > adding it now isn't likely to hurt. > > > > > > So I'll merge it now and shall aim to get it upstream very late in the > > > next merge window. > > > > Nope. mm.git won't build without acomp_is_async(). > > > > We can merge the zswap patch via the crypto tree. Acked-by: me. > > Herbert Acked the acomp_is_async() patch in v5 instead of picking it up > into crypto: > https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/ More details: Herbert acked the acomp_is_async in v5 [1], while he requested that patch 3/3 of v5 be split from the series and applied by crypto [2]. Patch 3/3 of v5 can function independently of 1/3 and 2/3, and it has already been included in the crypto tree. That is why v6 has only two left. [1] https://lore.kernel.org/linux-mm/ZdWKz43tTz2XY4ca@gondor.apana.org.au/ [2] https://lore.kernel.org/linux-mm/ZdWLim6zYSl%2Fx1Bk@gondor.apana.org.au/ > > > > > Or please just resend the zswap change after 6.9-rc1 is released. Thanks Barry
On Fri, Mar 8, 2024 at 8:11 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Thu, Feb 22, 2024 at 09:11:33PM +1300, Barry Song wrote: > > From: Barry Song <v-songbaohua@oppo.com> > > > > In zswap, if we use zsmalloc, we cannot sleep while we map the > > compressed memory, so we copy it to a temporary buffer. By > > knowing the alg won't sleep can help zswap to avoid the > > memcpy. > > Thus we introduce an API in crypto to expose if acomp is async, > > and zswap can use it to decide if it can remove copying to the > > tmp buffer. > > > > -v6: > > * add acked-by of Herbert, Thanks! > > * remove patch 3/3 from the series, as that one will go > > through crypto > > > > Barry Song (2): > > crypto: introduce: acomp_is_async to expose if comp drivers might > > sleep > > mm/zswap: remove the memcpy if acomp is not sleepable > > > > include/crypto/acompress.h | 6 ++++++ > > mm/zswap.c | 6 ++++-- > > 2 files changed, 10 insertions(+), 2 deletions(-) > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > Looks good to me. > > One small question: why cache is_sleepable in zswap instead of > checking acomp_is_async() directly? It doesn't look expensive. Thank you, Johannes. Your question sparked an idea for me. Drivers, such as drivers/crypto/intel/iaa/iaa_crypto_main.c, have the capability to dynamically switch between synchronous and asynchronous modes. Consequently, they also have the opportunity to dynamically modify cra_flags. This means that we may not require the cache. right now, iaa always has an ASYNC flag and needs a memcpy. If we can dynamically change the flag, the iaa platform might be able to further remove a memcpy in zswap if sync mode is set for it. /* * The iaa crypto driver supports three 'sync' methods determining how * compressions and decompressions are performed: * * - sync: the compression or decompression completes before * returning. This is the mode used by the async crypto * interface when the sync mode is set to 'sync' and by * the sync crypto interface regardless of setting. * * - async: the compression or decompression is submitted and returns * immediately. Completion interrupts are not used so * the caller is responsible for polling the descriptor * for completion. This mode is applicable to only the * async crypto interface and is ignored for anything * else. * * - async_irq: the compression or decompression is submitted and * returns immediately. Completion interrupts are * enabled so the caller can wait for the completion and * yield to other threads. When the compression or * decompression completes, the completion is signaled * and the caller awakened. This mode is applicable to * only the async crypto interface and is ignored for * anything else. * * These modes can be set using the iaa_crypto sync_mode driver * attribute. */ But it seems we will get lots of synchronized issues particularly when we are changing the mode by sysfs. static ssize_t sync_mode_store(struct device_driver *driver, const char *buf, size_t count) { ... } I don't have any iaa hardware. But I'd like to ask if this can interest Tom, the maintainer of INTEL IAA CRYPTO DRIVER to have a go :-) +Tom Zanussi Thanks Barry
From: Barry Song <v-songbaohua@oppo.com> In zswap, if we use zsmalloc, we cannot sleep while we map the compressed memory, so we copy it to a temporary buffer. By knowing the alg won't sleep can help zswap to avoid the memcpy. Thus we introduce an API in crypto to expose if acomp is async, and zswap can use it to decide if it can remove copying to the tmp buffer. -v6: * add acked-by of Herbert, Thanks! * remove patch 3/3 from the series, as that one will go through crypto Barry Song (2): crypto: introduce: acomp_is_async to expose if comp drivers might sleep mm/zswap: remove the memcpy if acomp is not sleepable include/crypto/acompress.h | 6 ++++++ mm/zswap.c | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-)