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@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
