802.1Q VLAN

net/8021q/vlan_dev.c +14 -5

@@ -484,13 +484,32 @@
 	       veth->h_vlan_proto, veth->h_vlan_TCI, 
 	       veth->h_vlan_encapsulated_proto);
 #endif
 
-	stats->tx_packets++; /* for statics only */
-	stats->tx_bytes += skb->len;
-
 	skb->dev = VLAN_DEV_INFO(dev)->real_dev;
-	dev_queue_xmit(skb);
 
-	return 0;
+	{
+		/* Please note, dev_queue_xmit consumes the pkt regardless of the
+		 * error value.  So, will copy the skb first and free if successful.
+		 */
+		struct sk_buff* skb2 = skb_get(skb);
+		int rv = dev_queue_xmit(skb2);
+		if (rv != 0) {
+			/* The skb memory should still be valid since we made a copy,
+			 * so can return error code here.
+			 */
+			return rv;
+		}
+		else {
+			/* Was success, need to free the skb reference since we bumped up the

Minor nits below.

[...]

Quoted 26 lines

net/8021q/vlan_dev.c +14 -5

@@ -484,13 +484,32 @@
 	       veth->h_vlan_proto, veth->h_vlan_TCI, 
 	       veth->h_vlan_encapsulated_proto);
 #endif
 
-	stats->tx_packets++; /* for statics only */
-	stats->tx_bytes += skb->len;
-
 	skb->dev = VLAN_DEV_INFO(dev)->real_dev;
-	dev_queue_xmit(skb);
 
-	return 0;
+	{
+		/* Please note, dev_queue_xmit consumes the pkt regardless of the
+		 * error value.  So, will copy the skb first and free if successful.
+		 */
+		struct sk_buff* skb2 = skb_get(skb);
+		int rv = dev_queue_xmit(skb2);
+		if (rv != 0) {
+			/* The skb memory should still be valid since we made a copy,
+			 * so can return error code here.
+			 */
+			return rv;
+		}
+		else {
+			/* Was success, need to free the skb reference since we bumped up the

net/8021q/vlan_dev.c line 513

Quoted 10 lines

net/8021q/vlan_dev.c +10 -0

+			 * user count above.
+			 */
+
+			stats->tx_packets++; /* for statics only */
+			stats->tx_bytes += skb->len;
+
+			kfree_skb(skb);
+			return 0;
+		}
+	}

Why not use a single return point, say:

		struct sk_buff *skb2 = skb_get(skb);
		int rv = dev_queue_xmit(skb2);

		if (!rv) {
			/* 
			 * Was success, need to free the skb reference since 
			 * we bumped up the user count above.
			 */

			stats->tx_packets++; /* for statics only */
			stats->tx_bytes += skb->len;

			kfree_skb(skb);
		}
		return rv;

vlan_dev_get_realdev_name() and vlan_dev_get_vid() can be tidy up a bit (factoring dev_put() or handling debug code differently for instance).

(quoted diff) +2 -1

@@ -1253,6 +1272,17 @@
  *	A negative errno code is returned on a failure. A success does not
  *	guarantee the frame will be transmitted as it may be dropped due
  *	to congestion or traffic shaping.
+ *
+ * 
-----------------------------------------------------------------------------------

Ok, I believe I have fixed all the suggestions offered by Francois (thanks!).

Please see what you think of this updated patch. The changes have been compile tested...and will pass some data in just a few minutes to make sure nothing funny happens...

Thanks,
Ben

DaveM, please consider accepting this patch (it's in a previous email to netdev, just ask if you want it re-sent.)

DaveM, please consider accepting this patch (it's in a previous email to netdev, just ask if you want it re-sent.)

Quoted 2 lines · On Mon, 25 Oct 2004 16:56:10 -0700 Ben Greear <[email protected]> wrote:

DaveM, please consider accepting this patch (it's in a previous email to netdev, just ask if you want it re-sent.)

Applied, but I had to apply it by hand. It gave a large reject. For some reason, patch barfed at the end of the vlan_dev.c parts and refused to interpreter the parts of the diffs for the other files.

Did you try to apply that patch to a tree before sending it off?

Anyways, I put it in by hand nevertheless.

Also, what is up with the magic "23" constant in the get realdev name function strncpy() call?

Quoted 2 lines

Also, what is up with the magic "23" constant in the get realdev name function strncpy() call?

Well, the fields are 24-bytes long, as defined in the include/linux/if_vlan.h file. I could change it to a #define and/or use sizeof if you prefer.

net/8021q/vlan_dev.c +12 -6

@@ -1,4 +1,4 @@
-/*
+/* -*- linux-c -*-
  * INET		802.1Q VLAN
  *		Ethernet-type device handling.
  *

@@ -484,13 +484,26 @@
 	       veth->h_vlan_proto, veth->h_vlan_TCI, veth->h_vlan_encapsulated_proto);
 #endif
 
-	stats->tx_packets++; /* for statics only */
-	stats->tx_bytes += skb->len;
-
 	skb->dev = VLAN_DEV_INFO(dev)->real_dev;
-	dev_queue_xmit(skb);
 
-	return 0;
+	{
+		/* Please note, dev_queue_xmit consumes the pkt regardless of the
+		 * error value.  So, will copy the skb first and free if successful.
+		 */
+		struct sk_buff* skb2 = skb_get(skb);
+		int rv = dev_queue_xmit(skb2);
+		if (rv == 0) {
+			/* Was success, need to free the skb reference since we bumped up the
+			 * user count above.
+			 */
+

Dave, I think this part of the patch should be reverted until someone comes up with a general scheme for flow_control handling through virtual devices.

Ben Greear wrote:

Hi Ben,

I am not happy with this change to vlan_dev_hard_start_xmit().

I certainly appreciate the idea of avoiding flow-control "black-holes", but that would take more than just this change. You have to consider (at least) these issues:

o It is considered an error if a queue-less device returns anything but
zero from its
hard_start_xmit() function (see dev_queue_xmit()).

o So, lets add a tx queue to it. Sure, that would be nice. Now we can
even do shaping
and other fancy stuff. But then how do we manage netif_queue_stopped?
Especially
restarting the queue could be tricky.

o But couldn't we skip netif_stop_queue() and just return
NETDEV_TX_BUSY when congested?
No, that would make the qdisc system "busy-retry" untill it succeeds.
BAD.

o It is unsafe to pass a shared skb to dev_queue_xmit() unless you
control all the
references yourself. (It will likely be enqueued on a list.)

And specifically for this patch:

o The skb could be freed (replaced) in __vlan_put_tag(), so you cannot
tell the caller
to hang on to it.

o If rv is NET_XMIT_CN (and probably also rv < 0) you have to return 0,
in order to
make the caller forget about this skb.

Right... it would probably be an O(N) thing to wake the queues for all virtual devices on a physical device, and we certainly don't want to do that often. Maybe if you only tried to wake the blocked queues (ie, kept a list of just blocked queues), then that would be less painful on average, but the worst-case is still bad.

Quoted 3 lines

o It is considered an error if a queue-less device returns anything but
zero from its
hard_start_xmit() function (see dev_queue_xmit()).

This certainly was not clear to me. The comments in dev_queue_xmit are wrong about the return value (failure cases can be > zero too). Are there other errors or ommissions there?

What sorts of things go wrong if you do return an error here when you don't have a queue?

Quoted 6 lines

o So, lets add a tx queue to it. Sure, that would be nice. Now we can
even do shaping
and other fancy stuff. But then how do we manage netif_queue_stopped?
Especially
restarting the queue could be tricky.

Right... it would probably be an O(N) thing to wake the queues for all virtual devices on a physical device, and we certainly don't want to do that often. Maybe if you only tried to wake the blocked queues (ie, kept a list of just blocked queues), then that would be less painful on average, but the worst-case is still bad.

Quoted 8 lines

o But couldn't we skip netif_stop_queue() and just return
NETDEV_TX_BUSY when congested?
No, that would make the qdisc system "busy-retry" untill it succeeds.
BAD.

o It is unsafe to pass a shared skb to dev_queue_xmit() unless you
control all the
references yourself. (It will likely be enqueued on a list.)

Since we either free the duplicate copy, or pass it to the queue and forget about it, this last point does not matter in the patch I submitted, right?

Quoted 5 lines

And specifically for this patch:

o The skb could be freed (replaced) in __vlan_put_tag(), so you cannot
tell the caller
to hang on to it.

Yep, that is quite nasty...I had not noticed. If I kept a copy of the original pointer (using skb_get() to bump the reference) passed in,
that would fix this particular problem?

Quoted 3 lines

o If rv is NET_XMIT_CN (and probably also rv < 0) you have to return 0,
in order to
make the caller forget about this skb.

Is there a complete list of what return codes are possible? Maybe we could make it return an enum instead of an integer so we can more easily track these sorts of things down??

Thanks for noticing!
Ben

Quoted 6 lines · Ben Greear

Quoted 2 lines

o It is considered an error if a queue-less device returns anything but zero from its hard_start_xmit() function (see dev_queue_xmit()).

This certainly was not clear to me. The comments in dev_queue_xmit are wrong about the return value (failure cases can be > zero too). Are there other errors or ommissions there?

A return value > zero doesn't mean failure. It indicates congestion.

Quoted 2 lines

What sorts of things go wrong if you do return an error here when you don't have a queue?

It is interpreted as a tx failure rather than congestion. So it doesn't help the upper layers like you wanted it to.
And it spews out an error message.

Quoted 5 lines

Quoted 4 lines

o So, lets add a tx queue to it. Sure, that would be nice. Now we can even do shaping and other fancy stuff. But then how do we manage netif_queue_stopped?
Especially
restarting the queue could be tricky.

Quoted 5 lines

Right... it would probably be an O(N) thing to wake the queues for all virtual devices on a physical device, and we certainly don't want to do that often. Maybe if you only tried to wake the blocked queues (ie, kept a list of just blocked queues), then that would be less painful on average, but the worst-case is still bad.

Yeah, we probably would need some sort of notification from the qdisc of the underlying device when it can accept packets again.

Quoted 7 lines

Quoted 6 lines

o But couldn't we skip netif_stop_queue() and just return NETDEV_TX_BUSY when congested? No, that would make the qdisc system "busy-retry" untill it succeeds.
BAD.

o It is unsafe to pass a shared skb to dev_queue_xmit() unless you control all the references yourself. (It will likely be enqueued on a list.)

Quoted 2 lines

Since we either free the duplicate copy, or pass it to the queue and forget about it, this last point does not matter in the patch I submitted, right?

Yes. This is the right way to do it. *Unless* the skb is already shared when you receive it (e.g. from pktgen).

Quoted 8 lines

Quoted 4 lines

And specifically for this patch:

o The skb could be freed (replaced) in __vlan_put_tag(), so you cannot tell the caller to hang on to it.

Yep, that is quite nasty...I had not noticed. If I kept a copy of the original pointer (using skb_get() to bump the reference) passed in,
that would fix this particular problem?

Yes, I would think so.

Quoted 11 lines · Tommy Christensen

Quoted 7 lines · Ben Greear

Quoted 2 lines

o It is considered an error if a queue-less device returns anything but zero from its hard_start_xmit() function (see dev_queue_xmit()).

This certainly was not clear to me. The comments in dev_queue_xmit are wrong about the return value (failure cases can be > zero too). Are there other errors or ommissions there?

Quoted 1 line

A return value > zero doesn't mean failure. It indicates congestion.

Ok, but the skb is always deleted by the net_queue_xmit code if the return is not zero? The difference between a hard-start-xmit failure on eth0 when the hardware-queue is full and having a rate-limiting queue drop a packet is virtually identical to me....

Quoted 4 lines

Quoted 2 lines

What sorts of things go wrong if you do return an error here when you don't have a queue?

Quoted 3 lines

It is interpreted as a tx failure rather than congestion. So it doesn't help the upper layers like you wanted it to.
And it spews out an error message.

The e1000 and probably other NICs have failed hard_start_xmit for a long time, and they are some of the most stable and high-performance NICs. So, the upper layers must be handling it OK some how or another.

Can you point me to some code that takes a different action based on the return values of dev_queue_xmit? That may help me understand better.

Quoted 13 lines

Quoted 11 lines

Quoted 4 lines

o So, lets add a tx queue to it. Sure, that would be nice. Now we can even do shaping and other fancy stuff. But then how do we manage
netif_queue_stopped? Especially
restarting the queue could be tricky.

Right... it would probably be an O(N) thing to wake the queues for all virtual devices on a physical device, and we certainly don't want to do that often. Maybe if you only tried to wake the blocked queues (ie, kept a list of just blocked queues), then that would be less painful on average, but the worst-case is still bad.

Quoted 2 lines

Yeah, we probably would need some sort of notification from the qdisc of the underlying device when it can accept packets again.

I did something like this for my non-busy-spin pktgen re-write and it works fine with both VLANs and physical devices. I just hooked directly into this code in netdevice.h:

static inline void netif_wake_queue(struct net_device *dev) {

#ifdef CONFIG_NETPOLL_TRAP
    if (netpoll_trap())
        return;
#endif
    if (test_and_clear_bit(__LINK_STATE_XOFF, &dev->state)) {
        __netif_schedule(dev);

                if (dev->notify_queue_woken) {
                   dev->notify_queue_woken(dev);
                }
        }
}

pktgen registers this hook on the physical device when it starts generating on the physical device or any VLANs attached to it. To make a scheme like this work in general, we'd probably need a chain of callbacks instead of a single method pointer...

Quoted 13 lines

Quoted 10 lines

Quoted 6 lines

o But couldn't we skip netif_stop_queue() and just return NETDEV_TX_BUSY when congested? No, that would make the qdisc system "busy-retry" untill it
succeeds. BAD.

o It is unsafe to pass a shared skb to dev_queue_xmit() unless you control all the references yourself. (It will likely be enqueued on a list.)

Since we either free the duplicate copy, or pass it to the queue and forget about it, this last point does not matter in the patch I submitted, right?

Quoted 2 lines

Yes. This is the right way to do it. *Unless* the skb is already shared when you receive it (e.g. from pktgen).

You can't send shared skbs regardless, because the vlan Xmit changes the skb->dev at least, so you just have to set the multi-skb setting in pktgen to 0 so that it does not share when using VLANs.

Quoted 3 lines · Ben Greear

You can't send shared skbs regardless, because the vlan Xmit changes the skb->dev at least, so you just have to set the multi-skb setting in pktgen to 0 so that it does not share when using VLANs.

By sheer accident, this would actually work! Nevertheless, the code should obviously handle this correctly (whatever that means?!).

Quoted 10 lines · Ben Greear

Quoted 1 line · Tommy Christensen

A return value > zero doesn't mean failure. It indicates congestion.

Ok, but the skb is always deleted by the net_queue_xmit code if the return is not zero? The difference between a hard-start-xmit failure on eth0 when the hardware-queue is full and having a rate-limiting queue drop a packet is virtually identical to me....

For a virtual device: yes, dev_queue_xmit() drops the skb. What else could it do with it? The semantic is that dev_queue_xmit always consumes skb's given to it.

A physical device will have a qdisc attached to it, so you don't get to see that the hardware queue is full. qdisc handles this case for you by retrying the transmission later. This is not (yet) congestion. OTOH if qdisc doesn't have room for a new skb in its *software* queue, the skb is dropped and congestion is reported upwards the stack.

Quoted 4 lines

pktgen registers this hook on the physical device when it starts generating on the physical device or any VLANs attached to it. To make a scheme like this work in general, we'd probably need a chain of callbacks instead of a single method pointer...

Nice. This idea is definitely worth persuing. However, ideally we would want to be notified when the *qdisc* queue opens up - this is our "tx ring buffer".

Can't you also add a queue to a VLAN device?

Quoted 3 lines

Nice. This idea is definitely worth persuing. However, ideally we would want to be notified when the *qdisc* queue opens up - this is our "tx ring buffer".

Maybe the qdisc could automatically flush what it could to lower level devices/queues whenever it was asked to enqueue a packet? This way, waking the writers could automatically wake the various queues under the writers.

That could be happening already, and might explain why my pktgen hacks work.

How does this look? I think it should fix the problem with having a new skb created in the __vlan_put_tag() method.