| Message ID | 1423536360-5298-1-git-send-email-brytonlee01@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) > >
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)