Re: [PATCH] 802.1Q VLAN

[Tommy Christensen <tommy.christensen@tpack.net>]

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