netdev - Re: [PATCH] snmp: add missing counters for RFC 4293

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090422184411.GE17947@hmsreliant.think-freely.org>
Date:	Wed, 22 Apr 2009 14:44:11 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Eric Dumazet <dada1@...mosbay.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] snmp: add missing counters for RFC 4293

On Wed, Apr 22, 2009 at 07:39:55PM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> > On Wed, Apr 22, 2009 at 02:50:03AM -0700, David Miller wrote:
> >> From: Eric Dumazet <dada1@...mosbay.com>
> >> Date: Wed, 22 Apr 2009 11:35:45 +0200
> >>
> >>> So RFC4293 tells corresponding Octets variables should be supported :
> >>>
> >>>  InOctets, InMcastOctets, OutMcastOctets, OutBcastOctets, OutOctets
> >>>
> >>> And I dont see them in /proc/net/snmp or /proc/net/netstat
> >>>
> >>> Mitsuru added :
> >>>
> >>>        SNMP_MIB_ITEM("InNoRoutes", IPSTATS_MIB_INNOROUTES),
> >>>        SNMP_MIB_ITEM("InTruncatedPkts", IPSTATS_MIB_INTRUNCATEDPKTS),
> >>>        SNMP_MIB_ITEM("InMcastPkts", IPSTATS_MIB_INMCASTPKTS),
> >>>        SNMP_MIB_ITEM("OutMcastPkts", IPSTATS_MIB_OUTMCASTPKTS),
> >>>        SNMP_MIB_ITEM("InBcastPkts", IPSTATS_MIB_INBCASTPKTS),
> >>>        SNMP_MIB_ITEM("OutBcastPkts", IPSTATS_MIB_OUTBCASTPKTS),
> >>>
> >>> And Neil adds : InOctets, OutOctets, InMcastOctets and OutMcastOctets
> >>>
> >>> Neil, you forgot OutBcastOctets :)
> >> Fair enough.  Neil I wait for an updated patch :-)
> >>
> > 
> > As promised, new patch, tested and ready to go.  This variant puts all the stats
> > at the end of the IpExt stats line in /proc/net/snmp, and at the end of
> > /proc/net/dev_snmp6/<iface>, so it shouldn't break any readers of those files.
> > 
> > Also, Eric, I know you mentioned the need for OutBcastOctets, and intuitively I
> > thought it should be there as well, but looking at RFC 4293, [In|Out]BcastOctets
> > isn't defined.  I started to wonder if perhaps the RFC assumed that McastOctets
> > was assumed to include broadcast frames (i.e. all hosts multicast), but I
> > decided that the smartest thing to do was correlate McastPkts and McastOctets.
> > If someone wants to make an argument for including broadcast explicitly in both
> > counters, then we can deal with that in a subsequent patch.
> 
> Yes, I found it on page 7, on diagram.
> 
> OutBcastPkts (1)  
> 
> Legend (1) is : (1) The HC counters and octet counters are also found at these points
>        but have been left out for clarity.
> 
> So apparently RFC forgot OutBcastOctets/InBcastOctets somewhere, oh well...
> 
> We actually have 
> 
> IpExt: InNoRoutes InTruncatedPkts InMcastPkts OutMcastPkts InBcastPkts OutBcastPkts
> 
> It would make sense to add : InOctets OutOctets InMcastOctets OutMcastOctets InBcastOctets OutBcastOctets
> Even if not fully tagged in RFC...
> 
I suppose, I'm just trying to avoid collecting data for stats that the snmp
agent doesn't recognize the oid for, but I suppose it couldn't hurt.

> 
> You could actually use new macros, to update both packet count and bytes count at once.
> (less calls to get_cpu()/put_cpu(), and per_cpu_ptr(), thats pretty expensive if CONFIG_PREEMPT
> 
> 
> #define SNMP_ADD_PB_STATS(mib, bytesfield, bytes, packetsfield) 	\
> 	do { 	__typeof__(mib) *ptr = per_cpu_ptr(mib[!in_softirq()], get_cpu()); \
> 		ptr->mibs[bytesfield] += bytes; \
> 		ptr->mibs[packetsfield]++; \
> 		put_cpu(); \
> 	} while (0)
> 
> #define SNMP_ADD_PB_STATS_BH(mib, bytesfield, bytes, packetsfield) 	\
> 	do { 	__typeof__(mib) *ptr = per_cpu_ptr(mib[0], get_cpu()); \
> 		ptr->mibs[bytesfield] += bytes; \
> 		ptr->mibs[packetsfield]++; \
> 		put_cpu(); \
> 	} while (0)
> 
> 
> PB as Packet & Bytes, maybe name is wrong :)
> 
Yeah, that makes sense, and I like the way it looks a bit better.  I'll update
the patch later this evening

Thanks!
Neil


> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html