* 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
[not found] ` <CGME20190603090232epcas5p1630d0584e8a1aa9495edc819605664fc@epcms5p1>
0 siblings, 2 replies; 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
[not found] ` <CGME20190603090232epcas5p1630d0584e8a1aa9495edc819605664fc@epcms5p1>
1 sibling, 0 replies; 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
[not found] ` <CGME20190603090232epcas5p1630d0584e8a1aa9495edc819605664fc@epcms5p1>
@ 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
Cc: Maninder Singh, herbert, davem, keescook, gustavo, joe,
linux-crypto, linux-kernel, AMIT SAHRAWAT, PANKAJ MISHRA,
Vaneet Narang, Chris Mason, Josef Bacik, David Sterba,
linux-btrfs, terrelln
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, Maninder Singh, herbert, davem, keescook,
gustavo, joe, linux-crypto, linux-kernel, AMIT SAHRAWAT,
PANKAJ MISHRA, Chris Mason, Josef Bacik, David Sterba,
linux-btrfs
> 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
[not found] ` <CGME20190603090232epcas5p1630d0584e8a1aa9495edc819605664fc@epcms5p1>
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;
as well as URLs for NNTP newsgroup(s).