* [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled
@ 2010-10-26 21:59 John Fastabend
2010-10-27 2:05 ` Jesse Gross
0 siblings, 1 reply; 4+ messages in thread
From: John Fastabend @ 2010-10-26 21:59 UTC (permalink / raw)
To: jesse; +Cc: john.r.fastabend, netdev, bhutchings
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>
---
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;
+ }
+
#if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
#endif
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled
2010-10-26 21:59 [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled John Fastabend
@ 2010-10-27 2:05 ` Jesse Gross
2010-10-27 21:40 ` John Fastabend
0 siblings, 1 reply; 4+ messages in thread
From: Jesse Gross @ 2010-10-27 2:05 UTC (permalink / raw)
To: John Fastabend; +Cc: netdev, bhutchings
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.
> ---
>
> 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled
2010-10-27 2:05 ` Jesse Gross
@ 2010-10-27 21:40 ` John Fastabend
2010-10-27 23:04 ` Jesse Gross
0 siblings, 1 reply; 4+ messages in thread
From: John Fastabend @ 2010-10-27 21:40 UTC (permalink / raw)
To: Jesse Gross; +Cc: netdev@vger.kernel.org, bhutchings@solarflare.com
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;
+
#if defined(CONFIG_FCOE) || defined(CONFIG_FCOE_MODULE)
vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
#endif
>> ---
>>
>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled
2010-10-27 21:40 ` John Fastabend
@ 2010-10-27 23:04 ` Jesse Gross
0 siblings, 0 replies; 4+ messages in thread
From: Jesse Gross @ 2010-10-27 23:04 UTC (permalink / raw)
To: John Fastabend; +Cc: netdev@vger.kernel.org, bhutchings@solarflare.com
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-10-27 23:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-26 21:59 [RFC][net-next-2.6 PATCH v2] 8021q: set hard_header_len when VLAN offload features are toggled John Fastabend
2010-10-27 2:05 ` Jesse Gross
2010-10-27 21:40 ` John Fastabend
2010-10-27 23:04 ` Jesse Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).