linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* 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 &params;
 }
 
 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).