[PATCH] prevent the read ahead of /proc/slabinfo in ss take 2

[PATCH] prevent the read ahead of /proc/slabinfo in ss take 2

[Bryton Lee]

ss reads ahead of /proc/slabinfo whatever slabstat will be used or not in future.
this will cause huge system delay when the kernel hires SLAB allocator(SLUB allocator is ok). when program reads
/proc/slabinfo, it will call s_show() in SLAB allocator level, and s_show() calls spin_lock_irq(&l3->list_lock)
then iterate on whole three lists. if one slab has about 900 million objects (for example dentry),
it will cause more than 1000ms delay.

so this patch prevents the read ahead of /proc/slabinfo, ss runs with most parameters not using slabstat at all.
and this patch also change slabstat and slabstat_valid to static.

Signed-off-by: Bryton Lee <brytonlee01@gmail.com>
---
 misc/ss.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 7fc0a99..5fa6259 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -616,7 +616,8 @@ struct slabstat
 	int skbs;
 };
 
-struct slabstat slabstat;
+static struct slabstat slabstat;
+static int slabstat_valid = 0;
 
 static const char *slabstat_ids[] =
 {
@@ -2230,6 +2231,9 @@ static int tcp_show(struct filter *f, int socktype)
 	 * it is able to give us some memory for snapshot.
 	 */
 	if (1) {
+		if (!slabstat_valid)
+			get_slabstat(&slabstat);
+
 		int guess = slabstat.socks+slabstat.tcp_syns;
 		if (f->states&(1<<SS_TIME_WAIT))
 			guess += slabstat.tcp_tws;
@@ -3196,6 +3200,9 @@ static int print_summary(void)
 	if (get_snmp_int("Tcp:", "CurrEstab", &sn.tcp_estab) < 0)
 		perror("ss: get_snmpstat");
 
+	if (!slabstat_valid)
+		get_slabstat(&slabstat);
+
 	printf("Total: %d (kernel %d)\n", s.socks, slabstat.socks);
 
 	printf("TCP:   %d (estab %d, closed %d, orphaned %d, synrecv %d, timewait %d/%d), ports %d\n",
@@ -3543,8 +3550,6 @@ int main(int argc, char *argv[])
 	argc -= optind;
 	argv += optind;
 
-	get_slabstat(&slabstat);
-
 	if (do_summary) {
 		print_summary();
 		if (do_default && argc == 0)
-- 
2.0.5

Re: [PATCH] prevent the read ahead of /proc/slabinfo in ss take 2

[Eric Dumazet]

On Tue, 2015-02-10 at 10:46 +0800, Bryton Lee wrote:
> ss reads ahead of /proc/slabinfo whatever slabstat will be used or not in future.
> this will cause huge system delay when the kernel hires SLAB allocator(SLUB allocator is ok). when program reads
> /proc/slabinfo, it will call s_show() in SLAB allocator level, and s_show() calls spin_lock_irq(&l3->list_lock)
> then iterate on whole three lists. if one slab has about 900 million objects (for example dentry),
> it will cause more than 1000ms delay.
> 

Please keep lengths < 70 chars in changelog.

> so this patch prevents the read ahead of /proc/slabinfo, ss runs with most parameters not using slabstat at all.
> and this patch also change slabstat and slabstat_valid to static.
> 
> Signed-off-by: Bryton Lee <brytonlee01@gmail.com>
> ---
>  misc/ss.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/misc/ss.c b/misc/ss.c
> index 7fc0a99..5fa6259 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -616,7 +616,8 @@ struct slabstat
>  	int skbs;
>  };
>  
> -struct slabstat slabstat;
> +static struct slabstat slabstat;
> +static int slabstat_valid = 0;

David pointed that you never set this variable to 1 or any value...

Why is this variable needed, if its content is known/constant ?


I presume you wanted to set it in get_slabstat(), right ?

>  
>  static const char *slabstat_ids[] =
>  {
> @@ -2230,6 +2231,9 @@ static int tcp_show(struct filter *f, int socktype)
>  	 * it is able to give us some memory for snapshot.
>  	 */
>  	if (1) {
> +		if (!slabstat_valid)
> +			get_slabstat(&slabstat);
> +
>  		int guess = slabstat.socks+slabstat.tcp_syns;
>  		if (f->states&(1<<SS_TIME_WAIT))
>  			guess += slabstat.tcp_tws;
> @@ -3196,6 +3200,9 @@ static int print_summary(void)
>  	if (get_snmp_int("Tcp:", "CurrEstab", &sn.tcp_estab) < 0)
>  		perror("ss: get_snmpstat");
>  
> +	if (!slabstat_valid)
> +		get_slabstat(&slabstat);
> +
>  	printf("Total: %d (kernel %d)\n", s.socks, slabstat.socks);
>  
>  	printf("TCP:   %d (estab %d, closed %d, orphaned %d, synrecv %d, timewait %d/%d), ports %d\n",
> @@ -3543,8 +3550,6 @@ int main(int argc, char *argv[])
>  	argc -= optind;
>  	argv += optind;
>  
> -	get_slabstat(&slabstat);
> -
>  	if (do_summary) {
>  		print_summary();
>  		if (do_default && argc == 0)

Re: [PATCH] prevent the read ahead of /proc/slabinfo in ss take 2

[Bryton Lee]

On Tue, Feb 10, 2015 at 12:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2015-02-10 at 10:46 +0800, Bryton Lee wrote:
>> ss reads ahead of /proc/slabinfo whatever slabstat will be used or not in future.
>> this will cause huge system delay when the kernel hires SLAB allocator(SLUB allocator is ok). when program reads
>> /proc/slabinfo, it will call s_show() in SLAB allocator level, and s_show() calls spin_lock_irq(&l3->list_lock)
>> then iterate on whole three lists. if one slab has about 900 million objects (for example dentry),
>> it will cause more than 1000ms delay.
>>
>
> Please keep lengths < 70 chars in changelog.
>
>> so this patch prevents the read ahead of /proc/slabinfo, ss runs with most parameters not using slabstat at all.
>> and this patch also change slabstat and slabstat_valid to static.
>>
>> Signed-off-by: Bryton Lee <brytonlee01@gmail.com>
>> ---
>>  misc/ss.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/misc/ss.c b/misc/ss.c
>> index 7fc0a99..5fa6259 100644
>> --- a/misc/ss.c
>> +++ b/misc/ss.c
>> @@ -616,7 +616,8 @@ struct slabstat
>>       int skbs;
>>  };
>>
>> -struct slabstat slabstat;
>> +static struct slabstat slabstat;
>> +static int slabstat_valid = 0;
>
> David pointed that you never set this variable to 1 or any value...
>
> Why is this variable needed, if its content is known/constant ?
>
>
> I presume you wanted to set it in get_slabstat(), right ?

Yes, you are right. I missed it when I merged code from my local
version to upstream, big thanks to you!  I will commit it soon.

>
>>
>>  static const char *slabstat_ids[] =
>>  {
>> @@ -2230,6 +2231,9 @@ static int tcp_show(struct filter *f, int socktype)
>>        * it is able to give us some memory for snapshot.
>>        */
>>       if (1) {
>> +             if (!slabstat_valid)
>> +                     get_slabstat(&slabstat);
>> +
>>               int guess = slabstat.socks+slabstat.tcp_syns;
>>               if (f->states&(1<<SS_TIME_WAIT))
>>                       guess += slabstat.tcp_tws;
>> @@ -3196,6 +3200,9 @@ static int print_summary(void)
>>       if (get_snmp_int("Tcp:", "CurrEstab", &sn.tcp_estab) < 0)
>>               perror("ss: get_snmpstat");
>>
>> +     if (!slabstat_valid)
>> +             get_slabstat(&slabstat);
>> +
>>       printf("Total: %d (kernel %d)\n", s.socks, slabstat.socks);
>>
>>       printf("TCP:   %d (estab %d, closed %d, orphaned %d, synrecv %d, timewait %d/%d), ports %d\n",
>> @@ -3543,8 +3550,6 @@ int main(int argc, char *argv[])
>>       argc -= optind;
>>       argv += optind;
>>
>> -     get_slabstat(&slabstat);
>> -
>>       if (do_summary) {
>>               print_summary();
>>               if (do_default && argc == 0)
>
>



-- 
Best Regards

Bryton.Lee

Re: [PATCH] prevent the read ahead of /proc/slabinfo in ss take 2

[Sergei Shtylyov]

Hello.

On 2/10/2015 5:46 AM, Bryton Lee wrote:

> ss reads ahead of /proc/slabinfo whatever slabstat will be used or not in future.
> this will cause huge system delay when the kernel hires SLAB allocator(SLUB allocator is ok). when program reads
> /proc/slabinfo, it will call s_show() in SLAB allocator level, and s_show() calls spin_lock_irq(&l3->list_lock)
> then iterate on whole three lists. if one slab has about 900 million objects (for example dentry),
> it will cause more than 1000ms delay.

> so this patch prevents the read ahead of /proc/slabinfo, ss runs with most parameters not using slabstat at all.
> and this patch also change slabstat and slabstat_valid to static.

> Signed-off-by: Bryton Lee <brytonlee01@gmail.com>
> ---
>   misc/ss.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

> diff --git a/misc/ss.c b/misc/ss.c
> index 7fc0a99..5fa6259 100644
> --- a/misc/ss.c
> +++ b/misc/ss.c
> @@ -616,7 +616,8 @@ struct slabstat
>   	int skbs;
>   };
>
> -struct slabstat slabstat;
> +static struct slabstat slabstat;
> +static int slabstat_valid = 0;

    No need to initialize to 0.

[...]

WBR, Sergei