[RFC,net-next-2.6,v2] 8021q: set hard_header_len when VLAN offload features are toggled

On 10/26/2010 7:05 PM, Jesse Gross wrote:
> On Tue, Oct 26, 2010 at 2:59 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
>> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
>> as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
>> This results in pskb_expand_head() being used unnecessarily.
>>
>> This add a check in vlan_transfer_features  to catch the ETH_FLAG_TXVLAN
>> flag and set the header length. This requires drivers to add the
>> ETH_FLAG_TXVLAN to vlan_features.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> I think this addresses all of the original problems.  However, I don't
> think that we want to have drivers claim to support vlan offloading as
> a feature for vlan packets.  That implies some type of QinQ
> functionality to me.  In addition, if the vlan device claims to
> support offloading and a second vlan device is stacked on top of it,
> then the two will clobber skb->vlan_tci.  It's probably simpler to
> just keep track of whether vlan offloading is currently enabled so we
> can find out whether it changed.
> 

Agreed. Rather then trying to be clever this is probably the easiest.



>> ---
>>
>>  net/8021q/vlan.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
>> index 05b867e..825011b 100644
>> --- a/net/8021q/vlan.c
>> +++ b/net/8021q/vlan.c
>> @@ -334,6 +334,16 @@ static void vlan_transfer_features(struct net_device *dev,
>>        vlandev->features &= ~dev->vlan_features;
>>        vlandev->features |= dev->features & dev->vlan_features;
>>        vlandev->gso_max_size = dev->gso_max_size;
>> +
>> +       /* is ETH_FLAGS_TXVLAN being toggled */
>> +       if ((vlandev->features & ETH_FLAG_TXVLAN) ^
>> +           (old_features & ETH_FLAG_TXVLAN)) {
>> +               if (vlandev->features & ETH_FLAG_TXVLAN)
>> +                       vlandev->hard_header_len -= VLAN_HLEN;
>> +               else
>> +                       vlandev->hard_header_len += VLAN_HLEN;
>> +       }
> 
> The correct flag for dev->features is NETIF_F_HW_VLAN_TX.
> ETH_FLAGS_TXVLAN is an ethtool construct (that happens to have the
> same value).
> 
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Oct 27, 2010 at 2:40 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 10/26/2010 7:05 PM, Jesse Gross wrote:
>> On Tue, Oct 26, 2010 at 2:59 PM, John Fastabend
>> <john.r.fastabend@intel.com> wrote:
>>> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len
>>> as well otherwise we end up using LL_RESERVED_SPACE incorrectly.
>>> This results in pskb_expand_head() being used unnecessarily.
>>>
>>> This add a check in vlan_transfer_features  to catch the ETH_FLAG_TXVLAN
>>> flag and set the header length. This requires drivers to add the
>>> ETH_FLAG_TXVLAN to vlan_features.
>>>
>>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>>
>> I think this addresses all of the original problems.  However, I don't
>> think that we want to have drivers claim to support vlan offloading as
>> a feature for vlan packets.  That implies some type of QinQ
>> functionality to me.  In addition, if the vlan device claims to
>> support offloading and a second vlan device is stacked on top of it,
>> then the two will clobber skb->vlan_tci.  It's probably simpler to
>> just keep track of whether vlan offloading is currently enabled so we
>> can find out whether it changed.
>>
>
> Agreed. Rather then trying to be clever this is probably the easiest.
>
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -334,6 +334,12 @@ Hunk #1, a/net/8021q/vlan.c static void vlan_transfer_features(struct net_device *dev,
>        vlandev->features &= ~dev->vlan_features;
>        vlandev->features |= dev->features & dev->vlan_features;
>        vlandev->gso_max_size = dev->gso_max_size;
> +
> +       if (dev->features & NETIF_F_HW_VLAN_TX)
> +               vlandev->hard_header_len = dev->hard_header_len;
> +       else
> +               vlandev->hard_header_len = dev->hard_header_len + VLAN_HLEN;
> +

Great, that's even simpler than I was thinking.

I think this series is ready to go.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html