[PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Mark Smith]

econet_rcv() calls ec_queue_packet(). The return from ec_queue_packet()
is the direct result of a call to sock_queue_rcv_skb(). Error returns
from ec_queue_packet() and therefore sock_queue_rcv_skb() are due to
kernel errors, so have econet_rcv() return NET_RX_BAD in this case.

If my understanding is correct, then

Signed-off-by: Mark Smith <markzzzsmith@yahoo.com.au>

diff --git a/net/econet/af_econet.c b/net/econet/af_econet.c
index 6f479fa..07f0f90 100644
--- a/net/econet/af_econet.c
+++ b/net/econet/af_econet.c
@@ -1092,8 +1092,10 @@ static int econet_rcv(struct sk_buff *skb, struct net_device *dev, struct packet
 		goto drop;
 
 	if (ec_queue_packet(sk, skb, edev->net, hdr->src_stn, hdr->cb,
-			    hdr->port))
-		goto drop;
+			    hdr->port)) {
+		kfree_skb(skb);
+		return NET_RX_BAD;
+	}
 
 	return 0;
 

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Florian Westphal]

Mark Smith <lk-netdev@lk-netdev.nosense.org> wrote:
> econet_rcv() calls ec_queue_packet(). The return from ec_queue_packet()
> is the direct result of a call to sock_queue_rcv_skb(). Error returns
> from ec_queue_packet() and therefore sock_queue_rcv_skb() are due to
> kernel errors, so have econet_rcv() return NET_RX_BAD in this case.

What about doing this instead?

From: Florian Westphal <fw@strlen.de>
Date: Sat, 20 Jun 2009 12:39:05 +0200
Subject: [PATCH] net: remove NET_RX_BAD and NET_RX_CN* defines

almost no users in the tree; and the few that use them treat them
like NET_RX_DROP.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 drivers/net/rionet.c            |    5 -----
 drivers/net/wan/farsync.c       |   19 -------------------
 drivers/staging/otus/wrap_pkt.c |    3 ---
 include/linux/netdevice.h       |    4 ----
 net/decnet/dn_route.c           |    2 +-
 net/lapb/lapb_iface.c           |    2 +-
 6 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/drivers/net/rionet.c b/drivers/net/rionet.c
index 8702e7a..74cdb6f 100644
--- a/drivers/net/rionet.c
+++ b/drivers/net/rionet.c
@@ -114,11 +114,6 @@ static int rionet_rx_clean(struct net_device *ndev)
 
 		if (error == NET_RX_DROP) {
 			ndev->stats.rx_dropped++;
-		} else if (error == NET_RX_BAD) {
-			if (netif_msg_rx_err(rnet))
-				printk(KERN_WARNING "%s: bad rx packet\n",
-				       DRV_NAME);
-			ndev->stats.rx_errors++;
 		} else {
 			ndev->stats.rx_packets++;
 			ndev->stats.rx_bytes += RIO_MAX_MSG_SIZE;
diff --git a/drivers/net/wan/farsync.c b/drivers/net/wan/farsync.c
index 25c9ef6..90c0a31 100644
--- a/drivers/net/wan/farsync.c
+++ b/drivers/net/wan/farsync.c
@@ -792,25 +792,6 @@ fst_process_rx_status(int rx_status, char *name)
 			 */
 			break;
 		}
-
-	case NET_RX_CN_LOW:
-		{
-			dbg(DBG_ASS, "%s: Receive Low Congestion\n", name);
-			break;
-		}
-
-	case NET_RX_CN_MOD:
-		{
-			dbg(DBG_ASS, "%s: Receive Moderate Congestion\n", name);
-			break;
-		}
-
-	case NET_RX_CN_HIGH:
-		{
-			dbg(DBG_ASS, "%s: Receive High Congestion\n", name);
-			break;
-		}
-
 	case NET_RX_DROP:
 		{
 			dbg(DBG_ASS, "%s: Received packet dropped\n", name);
diff --git a/drivers/staging/otus/wrap_pkt.c b/drivers/staging/otus/wrap_pkt.c
index 5db0004..89a6b92 100644
--- a/drivers/staging/otus/wrap_pkt.c
+++ b/drivers/staging/otus/wrap_pkt.c
@@ -156,10 +156,7 @@ void zfLnxRecvEth(zdev_t* dev, zbuf_t* buf, u16_t port)
     switch(netif_rx(buf))
 #endif
     {
-    case NET_RX_BAD:
     case NET_RX_DROP:
-    case NET_RX_CN_MOD:
-    case NET_RX_CN_HIGH:
         break;
     default:
             macp->drv_stats.net_stats.rx_packets++;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d4a4d98..9f25ab2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -72,10 +72,6 @@ struct wireless_dev;
 /* Backlog congestion levels */
 #define NET_RX_SUCCESS		0   /* keep 'em coming, baby */
 #define NET_RX_DROP		1  /* packet dropped */
-#define NET_RX_CN_LOW		2   /* storm alert, just in case */
-#define NET_RX_CN_MOD		3   /* Storm on its way! */
-#define NET_RX_CN_HIGH		4   /* The storm is here */
-#define NET_RX_BAD		5  /* packet dropped due to kernel error */
 
 /* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
  * indicates that the device will soon be dropping packets, or already drops
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 1d6ca8a..9383d3e 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -774,7 +774,7 @@ static int dn_rt_bug(struct sk_buff *skb)
 
 	kfree_skb(skb);
 
-	return NET_RX_BAD;
+	return NET_RX_DROP;
 }
 
 static int dn_rt_set_next_hop(struct dn_route *rt, struct dn_fib_res *res)
diff --git a/net/lapb/lapb_iface.c b/net/lapb/lapb_iface.c
index 2ba1bc4..bda96d1 100644
--- a/net/lapb/lapb_iface.c
+++ b/net/lapb/lapb_iface.c
@@ -407,7 +407,7 @@ int lapb_data_indication(struct lapb_cb *lapb, struct sk_buff *skb)
 		return lapb->callbacks.data_indication(lapb->dev, skb);
 
 	kfree_skb(skb);
-	return NET_RX_CN_HIGH; /* For now; must be != NET_RX_DROP */
+	return NET_RX_SUCCESS; /* For now; must be != NET_RX_DROP */
 }
 
 int lapb_data_transmit(struct lapb_cb *lapb, struct sk_buff *skb)
-- 
1.6.0.6

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Mark Smith]

Hi Florian,

On Sat, 20 Jun 2009 12:53:25 +0200
Florian Westphal <fw@strlen.de> wrote:

> Mark Smith <lk-netdev@lk-netdev.nosense.org> wrote:
> > econet_rcv() calls ec_queue_packet(). The return from ec_queue_packet()
> > is the direct result of a call to sock_queue_rcv_skb(). Error returns
> > from ec_queue_packet() and therefore sock_queue_rcv_skb() are due to
> > kernel errors, so have econet_rcv() return NET_RX_BAD in this case.
> 
> What about doing this instead?
> 

I think there is value in distinguishing between network/protocol
errors and kernel errors. It helps determine where the fault might lie
- in the network somewhere, or isolated to the receiving host. In
larger organisations there is typically a networks support team and a
hosts/sys admin team. Hints such as this that help determine who's
problem the fault is to deal with can be a big time saver (being a
networking person on one of these sorts of teams, I'm scrathing an
itch :-) )

If these econet patches are accepted, I'll send through similar patches
for the other protocols in the kernel where necessary.

Thanks,
Mark.

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[David Miller]

From: Mark Smith <lk-netdev@lk-netdev.nosense.org>
Date: Sat, 20 Jun 2009 20:50:26 +0930

> On Sat, 20 Jun 2009 12:53:25 +0200
> Florian Westphal <fw@strlen.de> wrote:
> 
>> Mark Smith <lk-netdev@lk-netdev.nosense.org> wrote:
>> > econet_rcv() calls ec_queue_packet(). The return from ec_queue_packet()
>> > is the direct result of a call to sock_queue_rcv_skb(). Error returns
>> > from ec_queue_packet() and therefore sock_queue_rcv_skb() are due to
>> > kernel errors, so have econet_rcv() return NET_RX_BAD in this case.
>> 
>> What about doing this instead?
> 
> I think there is value in distinguishing between network/protocol
> errors and kernel errors. It helps determine where the fault might lie
> - in the network somewhere, or isolated to the receiving host. In
> larger organisations there is typically a networks support team and a
> hosts/sys admin team. Hints such as this that help determine who's
> problem the fault is to deal with can be a big time saver (being a
> networking person on one of these sorts of teams, I'm scrathing an
> itch :-) )
> 
> If these econet patches are accepted, I'll send through similar patches
> for the other protocols in the kernel where necessary.

Indeed I bet whoever added NET_RX_BAD thought the distinction had
value too.

But it's been there for many years, and:

1) almost nobody sets it

2) nothing really acts upon or reports it specially

and if after all that time these two things are still true, then
it's obviously worthless in reality.

Therefore my inclination is to apply Florian's patch once the
merge window closes and the next-next-net-2.6 stuff starts getting
applied :-)

You can submit the econet stuff relative to that if you like.  But
I wonder Mark, are you actually using that protocol? :-)

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Mark Smith]

Hi Dave,

On Sun, 21 Jun 2009 22:50:18 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Mark Smith <lk-netdev@lk-netdev.nosense.org>
> Date: Sat, 20 Jun 2009 20:50:26 +0930
> 
> > On Sat, 20 Jun 2009 12:53:25 +0200
> > Florian Westphal <fw@strlen.de> wrote:
> > 
> >> Mark Smith <lk-netdev@lk-netdev.nosense.org> wrote:
> >> > econet_rcv() calls ec_queue_packet(). The return from ec_queue_packet()
> >> > is the direct result of a call to sock_queue_rcv_skb(). Error returns
> >> > from ec_queue_packet() and therefore sock_queue_rcv_skb() are due to
> >> > kernel errors, so have econet_rcv() return NET_RX_BAD in this case.
> >> 
> >> What about doing this instead?
> > 
> > I think there is value in distinguishing between network/protocol
> > errors and kernel errors. It helps determine where the fault might lie
> > - in the network somewhere, or isolated to the receiving host. In
> > larger organisations there is typically a networks support team and a
> > hosts/sys admin team. Hints such as this that help determine who's
> > problem the fault is to deal with can be a big time saver (being a
> > networking person on one of these sorts of teams, I'm scrathing an
> > itch :-) )
> > 
> > If these econet patches are accepted, I'll send through similar patches
> > for the other protocols in the kernel where necessary.
> 
> Indeed I bet whoever added NET_RX_BAD thought the distinction had
> value too.
> 
> But it's been there for many years, and:
> 
> 1) almost nobody sets it
> 
> 2) nothing really acts upon or reports it specially
> 
> and if after all that time these two things are still true, then
> it's obviously worthless in reality.
> 

It could be a bit of a chicken-and-egg problem. I didn't know there was
the option for the kernel to distinguish these errors until I
looked into the variety of NET_TX returns. Of course, the kernel doesn't
actually distinguish these types of errors because none of the protocols
(or at least the main ones I've looked at) specifically report them.

I'm not aware though as to currently where a general per-protocol count
of NET_RX_BADs vs NET_RX_DROPs might be exposed. I interpret the NIC
stats to be specific to the ethernet layer, so I think the dropped and
error counts for a NIC shouldn't be incremented for these layer 3
protocol errors. Having a brief look at the IPv4 and IPv6
implementations, they count these or similar errors in their MIB
counters.

Have a brief read through Neil Horman's dropwatch project page (I've
cc'd Neil), it would seem that the ideal place to report NET_RX_DROP vs
NET_RX_BAD would be via the netlink drop monitor protocol, exposing
packet drops between the generic incoming SKB and protocol handling
layer. Possibly more granularity could be added to report the error
type e.g. failed layer 3 checksum, incorrect version, socket queue full
etc.

> Therefore my inclination is to apply Florian's patch once the
> merge window closes and the next-next-net-2.6 stuff starts getting
> applied :-)
> 
> You can submit the econet stuff relative to that if you like.  But
> I wonder Mark, are you actually using that protocol? :-)
> 

Hah! I've used it once in my life. In the late 80s, BBC Microcomputers
were pretty propular here in Australia in schools, and my high school
had a network of them. Of course I don't think I even knew the word
protocol at the time, let alone which one we were using.

More recently though I did use the this econet implementation as one
of the things I looked at when I worked on ECTP a few months ago. I
looked around at appletalk, ipx, decnet, ipv4 and ipv6 to see examples
of the correct NET_RX return, which is when I noticed some
inconsistencies and also the lack of use of NET_RX_BAD where I thought
it should be used. (as an inconsistency example, in 2.6.30,
IPv6 is currently always returning 0, NET_RX_SUCCESS, even when the
packet is dropped.) I thought I'd have a go at cleaning up some of these
inconsistencies, with econet a very non-intrusive place to start.

Regards,
Mark.

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Sat, 20 Jun 2009 12:53:25 +0200

> Subject: [PATCH] net: remove NET_RX_BAD and NET_RX_CN* defines
> 
> almost no users in the tree; and the few that use them treat them
> like NET_RX_DROP.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied to net-next-2.6

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Mark Smith]

Hi Dave,

On Sun, 05 Jul 2009 19:47:05 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Florian Westphal <fw@strlen.de>
> Date: Sat, 20 Jun 2009 12:53:25 +0200
> 
> > Subject: [PATCH] net: remove NET_RX_BAD and NET_RX_CN* defines
> > 
> > almost no users in the tree; and the few that use them treat them
> > like NET_RX_DROP.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Applied to net-next-2.6

Will my first econet patch go in? It was just a "no magic numbers"
patch, unrelated to the NET_RX_DROP vs NET_RX_BAD issue.

Regards,
Mark.

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[David Miller]

From: Mark Smith <lk-netdev@lk-netdev.nosense.org>
Date: Mon, 6 Jul 2009 20:02:14 +0930

> Will my first econet patch go in? It was just a "no magic numbers"
> patch, unrelated to the NET_RX_DROP vs NET_RX_BAD issue.

I'm happy to apply that one, could you please resend it
to the list?  That would make my life easier, thanks!

Re: [PATCH] econet: have failed ec_queue_packet() call return NET_RX_BAD

[Mark Smith]

On Mon, 06 Jul 2009 11:48:52 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Mark Smith <lk-netdev@lk-netdev.nosense.org>
> Date: Mon, 6 Jul 2009 20:02:14 +0930
> 
> > Will my first econet patch go in? It was just a "no magic numbers"
> > patch, unrelated to the NET_RX_DROP vs NET_RX_BAD issue.
> 
> I'm happy to apply that one, could you please resend it
> to the list?  That would make my life easier, thanks!

Done, as a non-thread email.

Regards,
Mark.

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