Re: [patch net-next-2.6 37/47] igb: do vlan cleanup

[Jiri Pirko <jpirko@redhat.com>]

Thu, Jul 21, 2011 at 01:58:10AM CEST, jesse@nicira.com wrote:
>On Wed, Jul 20, 2011 at 12:10 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>> Wed, Jul 20, 2011 at 07:35:33PM CEST, jesse@nicira.com wrote:
>>>On Wed, Jul 20, 2011 at 7:54 AM, Jiri Pirko <jpirko@redhat.com> wrote:
>>>> @@ -2943,7 +2944,7 @@ static void igb_rlpml_set(struct igb_adapter *adapter)
>>>>        struct e1000_hw *hw = &adapter->hw;
>>>>        u16 pf_id = adapter->vfs_allocated_count;
>>>>
>>>> -       if (adapter->vlgrp)
>>>> +       if (igb_vlan_used(adapter))
>>>>                max_frame_size += VLAN_TAG_SIZE;
>>>
>>>There are similar issues here as with the VF driver.  I think you're
>>>also confusing vlan acceleration with vlan filtering.  If no vlan
>>>filters are in use but the card is in promiscuous mode, the buffer
>>>will be undersized and we lose tagged packets.
>>
>> I'm certainly not confusing vlan accel and filtering. Here is the
>> intension is the behaviour remains intact as well. I believe it's true.
>
>I believe the underlying issue for all three of these threads is the
>same, so I'll just respond to them all here.
>
>I agree that this doesn't change the behavior of the driver but I
>don't think that should be the goal.  When I originally designed this
>new vlan model my intention was to eliminate a whole class of driver
>bugs that I was repeatedly hitting in various forms.  In the example
>above, if you run tcpdump on this device without configuring a vlan
>group on it then you will see that MTU sized packets are missing
>because the receive buffer was undersized.
>
>The common theme for these problems is that they all occur in
>situations where vlans are not configured on the device and the driver
>does something different as a result of this.  The solution was to
>prevent drivers from changing their behavior in such situations by
>completely removing the concept of a vlan group from them and letting
>the networking core tell them when to make the changes instead of
>doing it implicitly.  That's why I don't see the fact that this change
>essentially emulates the knowledge of configuring a group to be a
>plus.  By the way, plenty of your other patches change the behavior of
>the drivers - on any of the NICs that always enable stripping, try
>running tcpdump on the interface without configuring a vlan group.
>Before the change you will see that tags have disappeared and
>afterwards the tags are intact.  So I think that changing the behavior
>of drivers in this regard is a positive thing.
>
>As an aside, thank you for taking the time to work on all of these
>drivers.  The only reason why I'm complaining about these few drivers
>is because I'd like to close the door on this class of problems, which
>is finally in reach thanks to your work.


Okay now it's clear to me. I tried to stay with the code as much similar
as unpatched. But I see your arguments. I will review and repost
patches which are enabling/disabling vlan accel on add_vid/kill_vid and
convert it to set_features.

Thanks. Jesse.

Jirka