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

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Apr 21, 2009 at 09:58:51PM +0200, Eric Dumazet wrote:
> Neil Horman a écrit :
> > The IP MIB (RFC 4293) defines stats for InOctets, OutOctets, InMcastOctets and
> > OutMcastOctets:
> > http://tools.ietf.org/html/rfc4293
> > But it seems we don't track those in any way that easy to separate from other
> > protocols.  This patch adds those missing counters to the stats file.  Tested
> > successfully by me
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > 
> >  include/linux/snmp.h  |    4 ++++
> >  net/ipv4/ip_input.c   |    4 ++++
> >  net/ipv4/ip_output.c  |    3 +++
> >  net/ipv4/proc.c       |    4 ++++
> >  net/ipv6/ip6_input.c  |    4 ++++
> >  net/ipv6/ip6_output.c |    8 ++++++++
> >  net/ipv6/mcast.c      |    9 +++++++++
> >  net/ipv6/ndisc.c      |    3 +++
> >  net/ipv6/proc.c       |    4 ++++
> >  net/ipv6/raw.c        |    6 ++++++
> >  10 files changed, 49 insertions(+)
> > 
> > diff --git a/include/linux/snmp.h b/include/linux/snmp.h
> > index aee3f1e..95c17f6 100644
> > --- a/include/linux/snmp.h
> > +++ b/include/linux/snmp.h
> > @@ -19,6 +19,8 @@ enum
> >  {
> >  	IPSTATS_MIB_NUM = 0,
> >  	IPSTATS_MIB_INRECEIVES,			/* InReceives */
> > +	IPSTATS_MIB_INOCTETS,			/* InOctets */
> > +	IPSTATS_MIB_INMCASTOCTETS,		/* InMcastOctets */
> >  	IPSTATS_MIB_INHDRERRORS,		/* InHdrErrors */
> >  	IPSTATS_MIB_INTOOBIGERRORS,		/* InTooBigErrors */
> >  	IPSTATS_MIB_INNOROUTES,			/* InNoRoutes */
> > @@ -29,6 +31,8 @@ enum
> >  	IPSTATS_MIB_INDELIVERS,			/* InDelivers */
> >  	IPSTATS_MIB_OUTFORWDATAGRAMS,		/* OutForwDatagrams */
> >  	IPSTATS_MIB_OUTREQUESTS,		/* OutRequests */
> > +	IPSTATS_MIB_OUTOCTETS,			/* OutOctets */
> > +	IPSTATS_MIB_OUTMCASTOCTETS,		/* OutMcastOctets */
> >  	IPSTATS_MIB_OUTDISCARDS,		/* OutDiscards */
> >  	IPSTATS_MIB_OUTNOROUTES,		/* OutNoRoutes */
> >  	IPSTATS_MIB_REASMTIMEOUT,		/* ReasmTimeout */
> > diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
> > index 1a58a6f..bc9169b 100644
> > --- a/net/ipv4/ip_input.c
> > +++ b/net/ipv4/ip_input.c
> > @@ -385,6 +385,7 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> >  		goto drop;
> >  
> >  	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INRECEIVES);
> > +	IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_INOCTETS, skb->len);
> >  
> >  	if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) {
> >  		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS);
> > @@ -396,6 +397,9 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt,
> >  
> >  	iph = ip_hdr(skb);
> >  
> > +	if (ipv4_is_multicast(iph->daddr))
> > +		IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_INMCASTOCTETS, skb->len);
> > +
> >  	/*
> >  	 *	RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
> >  	 *
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index 3e7e910..8a68dc2 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
> > @@ -245,6 +245,8 @@ int ip_mc_output(struct sk_buff *skb)
> >  	 *	If the indicated interface is up and running, send the packet.
> >  	 */
> 
> >  	IP_INC_STATS(dev_net(dev), IPSTATS_MIB_OUTREQUESTS);
> > +	IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_OUTOCTETS, skb->len);
> > +	IP_ADD_STATS_BH(dev_net(dev), IPSTATS_MIB_OUTMCASTOCTETS, skb->len);
> 
> So you use the _BH variant right after IP_INC_STATS() ?
> Which one is right (or which one is false ?)
> 

Both are correct (right now), at least as far as I can tell.  I'm not 100% sure why ADD
was named ADD_BH, except for the fact that it doesn't toggle the use of the mib
array passed in.  I think the right solution would be to simply rename
IP_ADD_STATS_BH to IP_ADD_STATS, and modify its implementation to access
mib[!in_softirq()] rather than just mib[0].  I had planned to do this in a
followup cleanup patch.  Alternatively, I could add a new IP_ADD_STATS to be
complete, but I don't really see the advantage (asside from not checking
in_softirq quite as often).

Same with all the other cases below.
Neil