Re: [patch net-next-2.6 v2] net: vlan: make non-hw-accel rx path similar to hw-accel

["Nicolas de Pesloüan" <nicolas.2p.debian@gmail.com>]

Le 21/05/2011 19:54, Jesse Gross a écrit :
> On Sat, May 21, 2011 at 6:17 AM, Nicolas de Pesloüan
> <nicolas.2p.debian@gmail.com>  wrote:
>> Le 21/05/2011 12:43, Changli Gao a écrit :
>>>
>>> On Sat, May 21, 2011 at 3:29 PM, Jiri Pirko<jpirko@redhat.com>    wrote:
>>>>
>>>> I do not see a reason why to not emulate that. To make paths as much
>>>> similar as they can be, that is the point of this patch.
>>>>
>>>> I think it would be better to fix an issue you are pointing at
>>>> rather that revert this.
>>>>
>>>
>>> In my opinion, the hardware accelerated VLAN RX is just a special case
>>> of the non hardware accelerated VLAN RX with header reordering. For
>>> promiscuous NICs and bridges, hw-accel-vlan-rx is just disabled.
>>
>> I strongly agree with that.
>>
>> The fact that a skb holds a VLAN tag is not a good enough reason to always
>> remove this tag before giving the skb to protocol handlers.
>>
>> If the user ask for VLAN tag removal, we should remove the tag, possibly
>> using hw-accel untagging if available else software untagging. And if the
>> user doesn't ask for tag removal, we should not untag.
>>
>> In other words, if the user doesn't setup any vlan interface on top of
>> another interface, there is no reason to untag the skb : both hw-accel
>> untagging and software untagging should be disabled.
>
> The problem is that for most hardware vlan stripping is actually the
> common case, not the exception.  When you try to disable it frequently
> there are hidden restrictions that cause problems.  A few examples:
> * Some NICs can't disable stripping at all.
> * Some NICs can only do tag insertion if stripping is configured on receive.
> * Some NICs can only do hardware offloads (checksum, TSO) if tag
> insertion is used on transmit.
>
> So if you are using vlans then acceleration is pretty much a fact of
> life and the best possible way we can deal with it is to make the
> accelerated and non-accelerated cases behave as similarly as possible.
>
> Before we were trying to dynamically enable/disable vlan acceleration
> based on whether a vlan group was configured and that worked fine for
> vlan devices because acceleration was enabled for it.  However, it
> caused an endless series of problems for other devices (such as
> bridging while trunking vlans) due to lost tags, driver bugs, and the
> restrictions above.  Some of these can be fixed with driver changes
> but the fact is that dynamically changing behavior just leads to
> problems for the less common cases that are supposedly being fixed.
> It's much better to do the same thing all the time.

Thanks for clarifying.

So, because many limited/buggy hardware exist, we must mimic the behavior in software. 'Sounds good 
to me.

And because some setups may still require the skb not to be untagged, may be we need the ability to 
re-tag the skb in some situations... When a protocol handler or rx_handler is explicitly registered 
on a net_device which expect to receive tagged skb, we should deliver tagged skb to it... Arguably, 
this may sound incredible for the general case, but may be required for not-so-special cases like 
bridge or protocol analyzer.

Of course, I don't say we should always re-tag: if no protocol handler nor rx_handler were 
registered on the parent interface, we don't need the extra work of re-tagging.

What I say is that it shouldn't be the job of protocol handlers or rx_handlers that expect the skb 
to be tagged to fix the improper untagging. A generic feature should do it when necessary.

And all this being said, it doesn't mean that we should pollute __netif_receive_skb with special 
code for vlan handling.

May be, as suggested by Eric W. Biederman in the V1 thread for this patch, software untagging for 
the first level of header should happen before __netif_receive_skb if we only try to mimic hardware 
behavior.

And possible later untagging (due to vlan nesting) should be done generically inside 
__netif_receive_skb, using rx_handler when appropriate. This would cleanup the general case where no 
vlan is involved at all.

	Nicolas.