diff mbox

udp: increment UDP_MIB_NOPORTS in mcast receive

Message ID 1349278153.12401.2811.camel@edumazet-glaptop
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Oct. 3, 2012, 3:29 p.m. UTC 
On Wed, 2012-10-03 at 10:09 -0400, David Stevens wrote:
> Eric Dumazet <eric.dumazet@gmail.com> wrote on 10/03/2012 09:15:51 AM:
>  
> > So when a host receives an UDP datagram but there was no application
> > at the destination port we should increment udpNoPorts, and its not
> > an error but just a fact.
> 
>         Of course. I think our difference is on the definition of 
> "receives".

A receive is a packet delivered to this host.
Interface being promiscuous or not doesnt really matter.

> I don't think a packet delivered locally due to promiscuous mode, 
> broadcast
> or an imperfect multicast address filter match is a host UDP datagram 
> receive.
> These packets really shouldn't be delivered to UDP at all; they are not
> addressed to this host (at least the non-broadcast, no-membership ones).

Thats the bug we currently are tracking. If some error is happening and
packet is delivered instead of being forwarded or dropped, we need a
counter being incremented to catch the bug.

>         A unicast UDP packet that doesn't match a local IP address does 
> not
> increment this counter. 

It _does_ increment this counter right now, not sure what you mean.

We currently correctly increment udpNoPorts if we receive an unicast UDP
packet that doesnt find a matching socket (because socket(s) are bound
to specific addresses instead of ANY_ADDR)

This is an extension of the "there was no application at the destination
port" to "there was no application at the destination port and
destination address"

> A promiscuous mode multicast delivery is no 
> different,
> except that the destination alone doesn't tell us if it is for us.
> 
>         I think counting these will primarily lead to administrators 
> seeing
> non-zero drops and wasting their time trying to track them down.

Well, as I said, seeing increments of this counter is perfectly fine and
matches RFC. It permits better diagnostics. Hiding bugs is not very
helpful.

Most of the time I am trying to track a bug in linux network stack, the
very first thing I ask to reporters is to post "netstat -s" before/after
their tests exactly because I want to see _some_ counters be incremented
and catch obvious problems.

And alas, many drops in our stack are not correctly reported because we
forgot to increment a counter at the right place.

I am fine adding a new SNMP McastDrops counter if you feel its better.

# grep Udp: /proc/net/snmp
Udp: InDatagrams NoPorts InErrors OutDatagrams RcvbufErrors SndbufErrors McastDrops
Udp: 11449164 15473 514616 290821178 0 184352 134

"netstat -s -u" would display :

Udp:
    11449164 packets received
    15473 packets to unknown port received.
    514616 packet receive errors
    290821178 packets sent
    SndbufErrors: 184352
    McastDrops: 134


Non official patch since net-next is not open :

 include/linux/snmp.h |    1 +
 net/ipv4/proc.c      |    1 +
 net/ipv4/udp.c       |    2 ++
 net/ipv6/proc.c      |    2 ++
 net/ipv6/udp.c       |    2 ++
 5 files changed, 8 insertions(+)




--
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

Comments

David Stevens Oct. 3, 2012, 5:31 p.m. UTC | #1 
Eric Dumazet <eric.dumazet@gmail.com> wrote on 10/03/2012 11:29:13 AM:

> >         Of course. I think our difference is on the definition of 
> > "receives".
> 
> A receive is a packet delivered to this host.
> Interface being promiscuous or not doesnt really matter.

        A receive is a packet *addressed* to this host. My point was
that running tcpdump/wireshark to look at other hosts' traffic
shouldn't affect any UDP MIB (these are ordinarily filtered by IP),
but I forgot that we are checking in software, as well as the HW multicast
address filter, for multicast group membership. So promiscuous mode and
imperfect NIC MAF hashes shouldn't actually result in local delivery
and that problem isn't there at all.

        I do think, still, that it is common to have broadcasts and
multicasts (for joined groups, even) with traffic completely uninteresting
to this host and that having a drop counter going up for those will
appear to be losses and errors when they are completely harmless and
irrelevant.
        But since it can't be incremented for items that are not actually
addressed to the local host, as I originally thought, I don't object
anymore. Sorry for the sidetrack -- I should've verified that originally.

                                                        +-DLS



--
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
David Miller Oct. 3, 2012, 7:30 p.m. UTC | #2 
From: David Stevens <dlstevens@us.ibm.com>
Date: Wed, 3 Oct 2012 13:31:30 -0400

> Eric Dumazet <eric.dumazet@gmail.com> wrote on 10/03/2012 11:29:13 AM:
> 
>> >         Of course. I think our difference is on the definition of 
>> > "receives".
>> 
>> A receive is a packet delivered to this host.
>> Interface being promiscuous or not doesnt really matter.
> 
>         A receive is a packet *addressed* to this host.

Although I'm largely ambivalent, this one sentence tipped me over
towards David's side on this issue.

But this is easy to resolve Eric, just simply make a new custom
counter that counts these new cases you care about and document it
properly.

Thanks.
--
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 mbox

Patch

diff --git a/include/linux/snmp.h b/include/linux/snmp.h
index 00bc189..321d643 100644
--- a/include/linux/snmp.h
+++ b/include/linux/snmp.h
@@ -145,6 +145,7 @@  enum
 	UDP_MIB_OUTDATAGRAMS,			/* OutDatagrams */
 	UDP_MIB_RCVBUFERRORS,			/* RcvbufErrors */
 	UDP_MIB_SNDBUFERRORS,			/* SndbufErrors */
+	UDP_MIB_MCASTDROPS,			/* McastDrops (linux extension) */
 	__UDP_MIB_MAX
 };
 
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 957acd1..1e932ee 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -172,6 +172,7 @@  static const struct snmp_mib snmp4_udp_list[] = {
 	SNMP_MIB_ITEM("OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("McastDrops", UDP_MIB_MCASTDROPS),
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 2814f66..4e2a4f7 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1591,6 +1591,8 @@  static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			sock_put(stack[i]);
 	} else {
 		kfree_skb(skb);
+		UDP_INC_STATS_BH(net, UDP_MIB_MCASTDROPS,
+				 udptable != &udp_table);
 	}
 	return 0;
 }
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 745a320..f2c12ea 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -129,6 +129,7 @@  static const struct snmp_mib snmp6_udp6_list[] = {
 	SNMP_MIB_ITEM("Udp6OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("Udp6RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("Udp6SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("Udp6McastDrops", UDP_MIB_MCASTDROPS),
 	SNMP_MIB_SENTINEL
 };
 
@@ -139,6 +140,7 @@  static const struct snmp_mib snmp6_udplite6_list[] = {
 	SNMP_MIB_ITEM("UdpLite6OutDatagrams", UDP_MIB_OUTDATAGRAMS),
 	SNMP_MIB_ITEM("UdpLite6RcvbufErrors", UDP_MIB_RCVBUFERRORS),
 	SNMP_MIB_ITEM("UdpLite6SndbufErrors", UDP_MIB_SNDBUFERRORS),
+	SNMP_MIB_ITEM("UdpLite6McastDrops", UDP_MIB_MCASTDROPS);
 	SNMP_MIB_SENTINEL
 };
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 07e2bfe..c8caf1b 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -748,6 +748,8 @@  static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			sock_put(stack[i]);
 	} else {
 		kfree_skb(skb);
+		UDP6_INC_STATS_BH(net, UDP_MIB_MCASTDROPS,
+				  udptable != &udp_table);
 	}
 	return 0;
 }