Patchwork [Open-FCoE,RFC,v2] fcoe: Use correct API to set vlan tag for FCoE Ethertype skbs

login
register
mail settings
Submitter Robert Love
Date June 18, 2013, 5:56 p.m.
Message ID <20130618175654.6271.53993.stgit@fritz>
Download mbox | patch
Permalink /patch/65/
State Superseded
Headers show

Comments

Robert Love - June 18, 2013, 5:56 p.m.
fcoe_xmit was coded such that it would skip the vlan net device/layer
and instead set some vlan flags and transmit on the real net device.
The real net device has code that would add the vlan tag for fcoe skbs.
This avoids some extra processing for data frames and provides a small
performance improvement.

Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
within the real net device's xmit routine was ultimately being
called to set the vlan tag.

With the below change the behavior of __vlan_put_tag changed slightly,
it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
being set by fcoe_xmit, so the skb->protocol is now not being set to
ETH_P_8021Q, as it should be.

This patch converts fcoe_xmit to use the vlan_put_tag routine which
will tag the skb and fcoe will continue to transmit fcoe skbs on the
real net device.

For reference, the below change was the one that altered the
__vlan_put_tag behavior.

  commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
  Author: Patrick McHardy <kaber@trash.net>
  Date:   Fri Apr 19 02:04:30 2013 +0000

      net: vlan: add protocol argument to packet tagging functions

      Add a protocol argument to the VLAN packet tagging functions. In case of HW
      tagging, we need that protocol available in the ndo_start_xmit functions,
      so it is stored in a new field in the skb. The new field fits into a hole
      (on 64 bit) and doesn't increase the sks's size.

Signed-off-by: Robert Love <robert.w.love@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
 drivers/scsi/fcoe/fcoe.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Vasu Dev - June 19, 2013, 5:16 p.m.
On Tue, 2013-06-18 at 10:56 -0700, Robert Love wrote:
> fcoe_xmit was coded such that it would skip the vlan net device/layer
> and instead set some vlan flags and transmit on the real net device.
> The real net device has code that would add the vlan tag for fcoe skbs.
> This avoids some extra processing for data frames and provides a small
> performance improvement.
> 

Given it was small performance gain and that too while ago, to my
recollection it was just 25K w/ only small size IOs. Now with newer
platforms this may not noticeable with significantly higher net IOPS.
Also skipping VLAN layer violates layering as patch adds VLAN tag while
using real dev, so instead I suggest just always use the device on which
FCoE exist and that would use vlan and real dev correctly according to
FCoE instance created on them to tag or not per underlying device in
use. 

This will make FCoE stack agonistic of VLAN or real device on TX path
and would also eliminate any such issue as this one fixed by the patch
in the future, I mean FCoE would go thru VLAN layer to inherit any
changes to vlan layer instead by-passing vlan layer and not aware of any
such changes to vlan layer/driver.

Thanks
Vasu

> Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
> within the real net device's xmit routine was ultimately being
> called to set the vlan tag.
> 
> With the below change the behavior of __vlan_put_tag changed slightly,
> it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
> being set by fcoe_xmit, so the skb->protocol is now not being set to
> ETH_P_8021Q, as it should be.
> 
> This patch converts fcoe_xmit to use the vlan_put_tag routine which
> will tag the skb and fcoe will continue to transmit fcoe skbs on the
> real net device.
> 
> For reference, the below change was the one that altered the
> __vlan_put_tag behavior.
> 
>   commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
>   Author: Patrick McHardy <kaber@trash.net>
>   Date:   Fri Apr 19 02:04:30 2013 +0000
> 
>       net: vlan: add protocol argument to packet tagging functions
> 
>       Add a protocol argument to the VLAN packet tagging functions. In case of HW
>       tagging, we need that protocol available in the ndo_start_xmit functions,
>       so it is stored in a new field in the skb. The new field fits into a hole
>       (on 64 bit) and doesn't increase the sks's size.
> 
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---
>  drivers/scsi/fcoe/fcoe.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index fc7bb1f..7efc5df 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1660,9 +1660,12 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
>  
>  	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
>  	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> -		skb->vlan_tci = VLAN_TAG_PRESENT |
> -				vlan_dev_vlan_id(fcoe->netdev);
> +		/* must set skb->dev before calling vlan_put_tag */
>  		skb->dev = fcoe->realdev;
> +		skb = vlan_put_tag(skb, htons(ETH_P_8021Q),
> +				   vlan_dev_vlan_id(fcoe->netdev));
> +		if (!skb)
> +			return -ENOMEM;
>  	} else
>  		skb->dev = fcoe->netdev;
Robert Love - June 19, 2013, 5:19 p.m.
> From: Vasu Dev [mailto:vasu.dev@linux.intel.com]
> Sent: Wednesday, June 19, 2013 10:17 AM
> To: Love, Robert W
> 
> On Tue, 2013-06-18 at 10:56 -0700, Robert Love wrote:
> > fcoe_xmit was coded such that it would skip the vlan net device/layer
> > and instead set some vlan flags and transmit on the real net device.
> > The real net device has code that would add the vlan tag for fcoe skbs.
> > This avoids some extra processing for data frames and provides a small
> > performance improvement.
> >
> 
> Given it was small performance gain and that too while ago, to my
> recollection it was just 25K w/ only small size IOs. Now with newer platforms
> this may not noticeable with significantly higher net IOPS.
> Also skipping VLAN layer violates layering as patch adds VLAN tag while using
> real dev, so instead I suggest just always use the device on which FCoE exist
> and that would use vlan and real dev correctly according to FCoE instance
> created on them to tag or not per underlying device in use.
> 
> This will make FCoE stack agonistic of VLAN or real device on TX path and
> would also eliminate any such issue as this one fixed by the patch in the
> future, I mean FCoE would go thru VLAN layer to inherit any changes to vlan
> layer instead by-passing vlan layer and not aware of any such changes to vlan
> layer/driver.

My thought regarding this was that since we're so late in the -rc cycle, I didn't
want to change the behavior, I only fix the failure.

I'm fine with transmitting using the VLAN device in the future especially if the
author of the VLAN device skipping is happy with going back to the VLAN device.

Thanks for the feedback. //Rob
Vasu Dev - June 19, 2013, 5:51 p.m.
On Wed, 2013-06-19 at 17:19 +0000, Love, Robert W wrote:
> > From: Vasu Dev [mailto:vasu.dev@linux.intel.com]
> > Sent: Wednesday, June 19, 2013 10:17 AM
> > To: Love, Robert W
> > 
> > On Tue, 2013-06-18 at 10:56 -0700, Robert Love wrote:
> > > fcoe_xmit was coded such that it would skip the vlan net device/layer
> > > and instead set some vlan flags and transmit on the real net device.
> > > The real net device has code that would add the vlan tag for fcoe skbs.
> > > This avoids some extra processing for data frames and provides a small
> > > performance improvement.
> > >
> > 
> > Given it was small performance gain and that too while ago, to my
> > recollection it was just 25K w/ only small size IOs. Now with newer platforms
> > this may not noticeable with significantly higher net IOPS.
> > Also skipping VLAN layer violates layering as patch adds VLAN tag while using
> > real dev, so instead I suggest just always use the device on which FCoE exist
> > and that would use vlan and real dev correctly according to FCoE instance
> > created on them to tag or not per underlying device in use.
> > 
> > This will make FCoE stack agonistic of VLAN or real device on TX path and
> > would also eliminate any such issue as this one fixed by the patch in the
> > future, I mean FCoE would go thru VLAN layer to inherit any changes to vlan
> > layer instead by-passing vlan layer and not aware of any such changes to vlan
> > layer/driver.
> 
> My thought regarding this was that since we're so late in the -rc cycle, I didn't
> want to change the behavior, I only fix the failure.
> 
> I'm fine with transmitting using the VLAN device in the future especially if the
> author of the VLAN device skipping is happy with going back to the VLAN device.
> 

I'm fine as author of that change as I'm suggesting to go thru vlan for
reasons listed above. Deferring is ok though it would be even smaller
change than the patch here to fix the issue as change is to simply use
fcoe netdev whether vlan or real dev instead added vlan_put_tag use by
the patch.

//Vasu
John Fastabend - June 21, 2013, 2:06 p.m.
On 6/18/2013 10:56 AM, Robert Love wrote:
> fcoe_xmit was coded such that it would skip the vlan net device/layer
> and instead set some vlan flags and transmit on the real net device.
> The real net device has code that would add the vlan tag for fcoe skbs.
> This avoids some extra processing for data frames and provides a small
> performance improvement.
>
> Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
> within the real net device's xmit routine was ultimately being
> called to set the vlan tag.
>
> With the below change the behavior of __vlan_put_tag changed slightly,
> it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
> being set by fcoe_xmit, so the skb->protocol is now not being set to
> ETH_P_8021Q, as it should be.
>
> This patch converts fcoe_xmit to use the vlan_put_tag routine which
> will tag the skb and fcoe will continue to transmit fcoe skbs on the
> real net device.
>
> For reference, the below change was the one that altered the
> __vlan_put_tag behavior.
>
>    commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
>    Author: Patrick McHardy <kaber@trash.net>
>    Date:   Fri Apr 19 02:04:30 2013 +0000
>
>        net: vlan: add protocol argument to packet tagging functions
>
>        Add a protocol argument to the VLAN packet tagging functions. In case of HW
>        tagging, we need that protocol available in the ndo_start_xmit functions,
>        so it is stored in a new field in the skb. The new field fits into a hole
>        (on 64 bit) and doesn't increase the sks's size.
>
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---

Acked-by: John Fastabend <john.r.fastabend@intel.com>


>   drivers/scsi/fcoe/fcoe.c |    7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index fc7bb1f..7efc5df 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1660,9 +1660,12 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
>
>   	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
>   	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> -		skb->vlan_tci = VLAN_TAG_PRESENT |
> -				vlan_dev_vlan_id(fcoe->netdev);
> +		/* must set skb->dev before calling vlan_put_tag */
>   		skb->dev = fcoe->realdev;
> +		skb = vlan_put_tag(skb, htons(ETH_P_8021Q),
> +				   vlan_dev_vlan_id(fcoe->netdev));
> +		if (!skb)
> +			return -ENOMEM;

Because you already check for NETIF_F_HW_VLAN_CTAG_TX use
__vlan_hwaccel_put_tag().

>   	} else
>   		skb->dev = fcoe->netdev;
>
>

Looks like a reasonable fix. A follow up patch could fix the if() to use
is_vlan_dev() and vlan_hw_offload_capable().

.John

Patch

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index fc7bb1f..7efc5df 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1660,9 +1660,12 @@  static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 
 	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
 	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
-		skb->vlan_tci = VLAN_TAG_PRESENT |
-				vlan_dev_vlan_id(fcoe->netdev);
+		/* must set skb->dev before calling vlan_put_tag */
 		skb->dev = fcoe->realdev;
+		skb = vlan_put_tag(skb, htons(ETH_P_8021Q),
+				   vlan_dev_vlan_id(fcoe->netdev));
+		if (!skb)
+			return -ENOMEM;
 	} else
 		skb->dev = fcoe->netdev;