Re: [PATCH] 802.1Q VLAN

[Ben Greear <greearb@candelatech.com>]

Tommy Christensen wrote:
> 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()).

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?

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

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


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

>  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

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