* Re: [PATCH 1/4] zstd: pass pointer rathen than structure to functions [not found] ` <1559552526-4317-2-git-send-email-maninder1.s@samsung.com> @ 2019-06-04 22:43 ` Andrew Morton 2019-06-05 11:57 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2019-06-04 22:43 UTC (permalink / raw) To: Maninder Singh Cc: herbert, davem, keescook, gustavo, joe, linux-crypto, linux-kernel, a.sahrawat, pankaj.m, v.narang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On Mon, 3 Jun 2019 14:32:03 +0530 Maninder Singh <maninder1.s@samsung.com> wrote: > currently params structure is passed in all functions, which increases > stack usage in all the function and lead to stack overflow on target like > ARM with kernel stack size of 8 KB so better to pass pointer. > > Checked for ARM: > > Original Patched > Call FLow Size: 1264 1040 > .... > (HUF_sort) -> 296 > (HUF_buildCTable_wksp) -> 144 > (HUF_compress4X_repeat) -> 88 > (ZSTD_compressBlock_internal) -> 200 > (ZSTD_compressContinue_internal)-> 136 -> 88 > (ZSTD_compressCCtx) -> 192 -> 64 > (zstd_compress) -> 144 -> 96 > (crypto_compress) -> 32 > (zcomp_compress) -> 32 > .... > > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > You missed btrfs. This needs review, please - particularly the kernel-wide static ZSTD_parameters in zstd_get_btrfs_parameters(). The base patch is here: http://lkml.kernel.org/r/1559552526-4317-2-git-send-email-maninder1.s@samsung.com --- a/fs/btrfs/zstd.c~zstd-pass-pointer-rathen-than-structure-to-functions-fix +++ a/fs/btrfs/zstd.c @@ -27,15 +27,17 @@ /* 307s to avoid pathologically clashing with transaction commit */ #define ZSTD_BTRFS_RECLAIM_JIFFIES (307 * HZ) -static ZSTD_parameters zstd_get_btrfs_parameters(unsigned int level, +static ZSTD_parameters *zstd_get_btrfs_parameters(unsigned int level, size_t src_len) { - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); + static ZSTD_parameters params; + + params = ZSTD_getParams(level, src_len, 0); if (params.cParams.windowLog > ZSTD_BTRFS_MAX_WINDOWLOG) params.cParams.windowLog = ZSTD_BTRFS_MAX_WINDOWLOG; WARN_ON(src_len > ZSTD_BTRFS_MAX_INPUT); - return params; + return ¶ms; } struct workspace { @@ -155,11 +157,12 @@ static void zstd_calc_ws_mem_sizes(void) unsigned int level; for (level = 1; level <= ZSTD_BTRFS_MAX_LEVEL; level++) { - ZSTD_parameters params = - zstd_get_btrfs_parameters(level, ZSTD_BTRFS_MAX_INPUT); - size_t level_size = - max_t(size_t, - ZSTD_CStreamWorkspaceBound(params.cParams), + ZSTD_parameters *params; + size_t level_size; + + params = zstd_get_btrfs_parameters(level, ZSTD_BTRFS_MAX_INPUT); + level_size = max_t(size_t, + ZSTD_CStreamWorkspaceBound(params->cParams), ZSTD_DStreamWorkspaceBound(ZSTD_BTRFS_MAX_INPUT)); max_size = max_t(size_t, max_size, level_size); @@ -385,8 +388,9 @@ static int zstd_compress_pages(struct li unsigned long len = *total_out; const unsigned long nr_dest_pages = *out_pages; unsigned long max_out = nr_dest_pages * PAGE_SIZE; - ZSTD_parameters params = zstd_get_btrfs_parameters(workspace->req_level, - len); + ZSTD_parameters *params; + + params = zstd_get_btrfs_parameters(workspace->req_level, len); *out_pages = 0; *total_out = 0; _ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] zstd: pass pointer rathen than structure to functions 2019-06-04 22:43 ` [PATCH 1/4] zstd: pass pointer rathen than structure to functions Andrew Morton @ 2019-06-05 11:57 ` David Sterba 2019-06-05 12:32 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2019-06-05 11:57 UTC (permalink / raw) To: Andrew Morton Cc: Maninder Singh, herbert, davem, keescook, gustavo, joe, linux-crypto, linux-kernel, a.sahrawat, pankaj.m, v.narang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs On Tue, Jun 04, 2019 at 03:43:26PM -0700, Andrew Morton wrote: > On Mon, 3 Jun 2019 14:32:03 +0530 Maninder Singh <maninder1.s@samsung.com> wrote: > > > currently params structure is passed in all functions, which increases > > stack usage in all the function and lead to stack overflow on target like > > ARM with kernel stack size of 8 KB so better to pass pointer. > > > > Checked for ARM: > > > > Original Patched > > Call FLow Size: 1264 1040 > > .... > > (HUF_sort) -> 296 > > (HUF_buildCTable_wksp) -> 144 > > (HUF_compress4X_repeat) -> 88 > > (ZSTD_compressBlock_internal) -> 200 > > (ZSTD_compressContinue_internal)-> 136 -> 88 > > (ZSTD_compressCCtx) -> 192 -> 64 > > (zstd_compress) -> 144 -> 96 > > (crypto_compress) -> 32 > > (zcomp_compress) -> 32 > > .... > > > > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > > > > You missed btrfs. This needs review, please - particularly the > kernel-wide static ZSTD_parameters in zstd_get_btrfs_parameters(). > > The base patch is here: > > http://lkml.kernel.org/r/1559552526-4317-2-git-send-email-maninder1.s@samsung.com > > --- a/fs/btrfs/zstd.c~zstd-pass-pointer-rathen-than-structure-to-functions-fix > +++ a/fs/btrfs/zstd.c > @@ -27,15 +27,17 @@ > /* 307s to avoid pathologically clashing with transaction commit */ > #define ZSTD_BTRFS_RECLAIM_JIFFIES (307 * HZ) > > -static ZSTD_parameters zstd_get_btrfs_parameters(unsigned int level, > +static ZSTD_parameters *zstd_get_btrfs_parameters(unsigned int level, > size_t src_len) > { > - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); > + static ZSTD_parameters params; > + > + params = ZSTD_getParams(level, src_len, 0); No thats' broken, the params can't be static as it depends on level and src_len. What happens if there are several requests in parallel with eg. different levels? Would be really great if the mailinglist is CCed when the code is changed in a non-trivial way. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] zstd: pass pointer rathen than structure to functions 2019-06-05 11:57 ` David Sterba @ 2019-06-05 12:32 ` David Sterba 2019-06-05 21:32 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2019-06-05 12:32 UTC (permalink / raw) To: Andrew Morton, Maninder Singh, herbert, davem, keescook, gustavo, joe, linux-crypto, linux-kernel, a.sahrawat, pankaj.m, v.narang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs, terrelln On Wed, Jun 05, 2019 at 01:57:03PM +0200, David Sterba wrote: > On Tue, Jun 04, 2019 at 03:43:26PM -0700, Andrew Morton wrote: > > On Mon, 3 Jun 2019 14:32:03 +0530 Maninder Singh <maninder1.s@samsung.com> wrote: > > > > > currently params structure is passed in all functions, which increases > > > stack usage in all the function and lead to stack overflow on target like > > > ARM with kernel stack size of 8 KB so better to pass pointer. > > > > > > Checked for ARM: > > > > > > Original Patched > > > Call FLow Size: 1264 1040 > > > .... > > > (HUF_sort) -> 296 > > > (HUF_buildCTable_wksp) -> 144 > > > (HUF_compress4X_repeat) -> 88 > > > (ZSTD_compressBlock_internal) -> 200 > > > (ZSTD_compressContinue_internal)-> 136 -> 88 > > > (ZSTD_compressCCtx) -> 192 -> 64 > > > (zstd_compress) -> 144 -> 96 > > > (crypto_compress) -> 32 > > > (zcomp_compress) -> 32 > > > .... > > > > > > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > > > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > > > > > > > You missed btrfs. This needs review, please - particularly the > > kernel-wide static ZSTD_parameters in zstd_get_btrfs_parameters(). > > > > > The base patch is here: > > > > http://lkml.kernel.org/r/1559552526-4317-2-git-send-email-maninder1.s@samsung.com > > > > --- a/fs/btrfs/zstd.c~zstd-pass-pointer-rathen-than-structure-to-functions-fix > > +++ a/fs/btrfs/zstd.c > > @@ -27,15 +27,17 @@ > > /* 307s to avoid pathologically clashing with transaction commit */ > > #define ZSTD_BTRFS_RECLAIM_JIFFIES (307 * HZ) > > > > -static ZSTD_parameters zstd_get_btrfs_parameters(unsigned int level, > > +static ZSTD_parameters *zstd_get_btrfs_parameters(unsigned int level, > > size_t src_len) > > { > > - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); > > + static ZSTD_parameters params; > > > + > > + params = ZSTD_getParams(level, src_len, 0); > > No thats' broken, the params can't be static as it depends on level and > src_len. What happens if there are several requests in parallel with > eg. different levels? > > Would be really great if the mailinglist is CCed when the code is > changed in a non-trivial way. So this does not compile fs/btrfs/zstd.o which Andrew probably found too, otherwise btrfs is the only in-tree user of the function outside of lib/ and crypto/. I think that Nick Terrell should have been CCed too, as he ported zstd to linux. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/4] zstd: pass pointer rathen than structure to functions 2019-06-05 12:32 ` David Sterba @ 2019-06-05 21:32 ` Andrew Morton 2019-06-06 14:10 ` Vaneet Narang 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2019-06-05 21:32 UTC (permalink / raw) To: dsterba Cc: Maninder Singh, herbert, davem, keescook, gustavo, joe, linux-crypto, linux-kernel, a.sahrawat, pankaj.m, v.narang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs, terrelln On Wed, 5 Jun 2019 14:32:53 +0200 David Sterba <dsterba@suse.cz> wrote: > > > > > > -static ZSTD_parameters zstd_get_btrfs_parameters(unsigned int level, > > > +static ZSTD_parameters *zstd_get_btrfs_parameters(unsigned int level, > > > size_t src_len) > > > { > > > - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); > > > + static ZSTD_parameters params; > > > > > + > > > + params = ZSTD_getParams(level, src_len, 0); > > > > No thats' broken, the params can't be static as it depends on level and > > src_len. What happens if there are several requests in parallel with > > eg. different levels? I wondered. I'll drop the patch series as some more serious thinking is needed. > > Would be really great if the mailinglist is CCed when the code is > > changed in a non-trivial way. Well we didn't actually change btrfs until I discovered that Maninder had missed it. > So this does not compile fs/btrfs/zstd.o which Andrew probably found > too, otherwise btrfs is the only in-tree user of the function outside of > lib/ and crypto/. Worked for me - I might have sent the wrong version. > I think that Nick Terrell should have been CCed too, as he ported zstd > to linux. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE:(2) [PATCH 1/4] zstd: pass pointer rathen than structure to functions 2019-06-05 21:32 ` Andrew Morton @ 2019-06-06 14:10 ` Vaneet Narang 2019-06-06 20:14 ` (2) " Nick Terrell 0 siblings, 1 reply; 6+ messages in thread From: Vaneet Narang @ 2019-06-06 14:10 UTC (permalink / raw) To: Andrew Morton, dsterba@suse.cz Cc: Maninder Singh, herbert@gondor.apana.org.au, davem@davemloft.net, keescook@chromium.org, gustavo@embeddedor.com, joe@perches.com, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, AMIT SAHRAWAT, PANKAJ MISHRA, Vaneet Narang, Chris Mason, Josef Bacik, David Sterba, linux-btrfs@vger.kernel.org, terrelln@fb.com Hi Andrew / David, >> > > - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); >> > > + static ZSTD_parameters params; >> > >> > > + >> > > + params = ZSTD_getParams(level, src_len, 0); >> > >> > No thats' broken, the params can't be static as it depends on level and >> > src_len. What happens if there are several requests in parallel with >> > eg. different levels? There is no need to make static for btrfs. We can keep it as a stack variable. This patch set focussed on reducing stack usage of zstd compression when triggered through zram. ZRAM internally uses crypto and currently crpto uses fixed level and also not dependent upon source length. crypto/zstd.c: static ZSTD_parameters zstd_params(void) { return ZSTD_getParams(ZSTD_DEF_LEVEL, 0, 0); } Actually high stack usage problem with zstd compression patch gets exploited more incase of shrink path which gets triggered randomly from any call flow in case of low memory and adds overhead of more than 2000 byte of stack and results in stack overflow. Stack usage of alloc_page in case of low memory 72 HUF_compressWeights_wksp+0x140/0x200 64 HUF_writeCTable_wksp+0xdc/0x1c8 88 HUF_compress4X_repeat+0x214/0x450 208 ZSTD_compressBlock_internal+0x224/0x137c 136 ZSTD_compressContinue_internal+0x210/0x3b0 192 ZSTD_compressCCtx+0x6c/0x144 144 zstd_compress+0x40/0x58 32 crypto_compress+0x2c/0x34 32 zcomp_compress+0x3c/0x44 80 zram_bvec_rw+0x2f8/0xa7c 64 zram_rw_page+0x104/0x170 48 bdev_write_page+0x80/0xb4 112 __swap_writepage+0x160/0x29c 24 swap_writepage+0x3c/0x58 160 shrink_page_list+0x788/0xae0 128 shrink_inactive_list+0x210/0x4a8 184 shrink_zone+0x53c/0x7c0 160 try_to_free_pages+0x2fc/0x7cc 80 __alloc_pages_nodemask+0x534/0x91c Thanks & Regards, Vaneet Narang ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (2) [PATCH 1/4] zstd: pass pointer rathen than structure to functions 2019-06-06 14:10 ` Vaneet Narang @ 2019-06-06 20:14 ` Nick Terrell 0 siblings, 0 replies; 6+ messages in thread From: Nick Terrell @ 2019-06-06 20:14 UTC (permalink / raw) To: Vaneet Narang Cc: Andrew Morton, dsterba@suse.cz, Maninder Singh, herbert@gondor.apana.org.au, davem@davemloft.net, keescook@chromium.org, gustavo@embeddedor.com, joe@perches.com, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, AMIT SAHRAWAT, PANKAJ MISHRA, Chris Mason, Josef Bacik, David Sterba, linux-btrfs@vger.kernel.org > On Jun 6, 2019, at 7:10 AM, Vaneet Narang <v.narang@samsung.com> wrote: > > Hi Andrew / David, > > >>> > > - ZSTD_parameters params = ZSTD_getParams(level, src_len, 0); >>> > > + static ZSTD_parameters params; >>> > >>> > > + >>> > > + params = ZSTD_getParams(level, src_len, 0); >>> > >>> > No thats' broken, the params can't be static as it depends on level and >>> > src_len. What happens if there are several requests in parallel with >>> > eg. different levels? > > There is no need to make static for btrfs. We can keep it as a stack variable. > This patch set focussed on reducing stack usage of zstd compression when triggered > through zram. ZRAM internally uses crypto and currently crpto uses fixed level and also > not dependent upon source length. Can we measure the performance of these patches on btrfs and/or zram? See the benchmarks I ran on my original patch to btrfs for reference https://lore.kernel.org/patchwork/patch/802866/. I don't expect a speed difference, but I think it is worth measuring. > crypto/zstd.c: > static ZSTD_parameters zstd_params(void) > { > return ZSTD_getParams(ZSTD_DEF_LEVEL, 0, 0); > } > > > Actually high stack usage problem with zstd compression patch gets exploited more incase of > shrink path which gets triggered randomly from any call flow in case of low memory and adds overhead > of more than 2000 byte of stack and results in stack overflow. > > Stack usage of alloc_page in case of low memory > > 72 HUF_compressWeights_wksp+0x140/0x200 > 64 HUF_writeCTable_wksp+0xdc/0x1c8 > 88 HUF_compress4X_repeat+0x214/0x450 > 208 ZSTD_compressBlock_internal+0x224/0x137c > 136 ZSTD_compressContinue_internal+0x210/0x3b0 > 192 ZSTD_compressCCtx+0x6c/0x144 > 144 zstd_compress+0x40/0x58 > 32 crypto_compress+0x2c/0x34 > 32 zcomp_compress+0x3c/0x44 > 80 zram_bvec_rw+0x2f8/0xa7c > 64 zram_rw_page+0x104/0x170 > 48 bdev_write_page+0x80/0xb4 > 112 __swap_writepage+0x160/0x29c > 24 swap_writepage+0x3c/0x58 > 160 shrink_page_list+0x788/0xae0 > 128 shrink_inactive_list+0x210/0x4a8 > 184 shrink_zone+0x53c/0x7c0 > 160 try_to_free_pages+0x2fc/0x7cc > 80 __alloc_pages_nodemask+0x534/0x91c > > Thanks & Regards, > Vaneet Narang > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-06 20:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1559552526-4317-1-git-send-email-maninder1.s@samsung.com>
[not found] ` <CGME20190603090232epcas5p1630d0584e8a1aa9495edc819605664fc@epcas5p1.samsung.com>
[not found] ` <1559552526-4317-2-git-send-email-maninder1.s@samsung.com>
2019-06-04 22:43 ` [PATCH 1/4] zstd: pass pointer rathen than structure to functions Andrew Morton
2019-06-05 11:57 ` David Sterba
2019-06-05 12:32 ` David Sterba
2019-06-05 21:32 ` Andrew Morton
2019-06-06 14:10 ` Vaneet Narang
2019-06-06 20:14 ` (2) " Nick Terrell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox