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.
> 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.
>
> > 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.
> > 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).
> > 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.
> > 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??
They are listed in netdevice.h - NET_XMIT_SUCCESS etc., and the usual
negative errno's.
> Thanks for noticing!
> Ben
-Tommy
|