Re: [PATCH] 802.1Q VLAN

[Ben Greear <greearb@candelatech.com>]

Tommy Christensen wrote:
> On Fri, 2004-10-29 at 02:23, Ben Greear wrote:
> 
>>> 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.

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

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

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.

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


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

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

I will test this change and send a follow-up patch if it proves stable.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com