[PATCH net-next 0/7] net: dsa: tagger simplification

[PATCH net-next 0/7] net: dsa: tagger simplification

[Vivien Didelot]

The DSA layer has a dsa_device_ops structure containing two members to
tag and untag the proprietary switch header, called xmit and rcv.

The switch tree and slave private structures respectively hold a useless
copy of the rcv and xmit functions. The tagging implementations use
useless goto labels and only the rcv caller (partially) handles the SKB
freeing. The rcv function also contains unused arguments.

This patchset removes the copy of rcv and xmit functions, the unused
arguments of the rcv signature and the useless labels, and handles
freeing of the SKB in the xmit caller.

Vivien Didelot (7):
  net: dsa: hide dsa_uses_tagged_protocol code
  net: dsa: remove useless rcv copy in DSA tree
  net: dsa: remove unused arguments of tagger rcv
  net: dsa: free orig skb on rcv if reallocated
  net: dsa: remove useless goto label in tagger rcv
  net: dsa: remove useless copy of tagger xmit
  net: dsa: factor skb freeing on xmit

 include/net/dsa.h     |  9 +--------
 net/dsa/dsa.c         | 12 +++++++++---
 net/dsa/dsa2.c        |  2 --
 net/dsa/dsa_priv.h    |  7 +------
 net/dsa/legacy.c      |  2 --
 net/dsa/slave.c       |  6 +++---
 net/dsa/tag_brcm.c    | 21 ++++++---------------
 net/dsa/tag_dsa.c     | 25 ++++++++-----------------
 net/dsa/tag_edsa.c    | 25 ++++++++-----------------
 net/dsa/tag_lan9303.c |  8 ++------
 net/dsa/tag_mtk.c     | 19 +++++--------------
 net/dsa/tag_qca.c     | 21 ++++++---------------
 net/dsa/tag_trailer.c | 18 +++++-------------
 13 files changed, 54 insertions(+), 121 deletions(-)

-- 
2.13.0

[PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[Vivien Didelot]

Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this
helper will be extended to access the opaque dsa_device_ops structure.

At the same time, fix the checkpatch comparison check:

    CHECK: Comparison to NULL could be written "dst->rcv"
    #41: FILE: net/dsa/dsa.c:32:
    +	return dst->rcv != NULL;

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h | 5 +----
 net/dsa/dsa.c     | 5 +++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index c0e567c0c824..cb5d668b265d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
 
 struct net_device *dsa_dev_to_net_device(struct device *dev);
 
-static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
-{
-	return dst->rcv != NULL;
-}
+bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst);
 
 static inline bool netdev_uses_dsa(struct net_device *dev)
 {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 3288a80d4d6c..7a8a0358299b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -27,6 +27,11 @@
 
 #include "dsa_priv.h"
 
+bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
+{
+	return !!dst->rcv;
+}
+
 static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb,
 					    struct net_device *dev)
 {
-- 
2.13.0

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:25AM -0400, Vivien Didelot wrote:
> Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this
> helper will be extended to access the opaque dsa_device_ops structure.
> 
> At the same time, fix the checkpatch comparison check:
> 
>     CHECK: Comparison to NULL could be written "dst->rcv"
>     #41: FILE: net/dsa/dsa.c:32:
>     +	return dst->rcv != NULL;
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  include/net/dsa.h | 5 +----
>  net/dsa/dsa.c     | 5 +++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index c0e567c0c824..cb5d668b265d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
>  
>  struct net_device *dsa_dev_to_net_device(struct device *dev);
>  
> -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
> -{
> -	return dst->rcv != NULL;
> -}
> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst);
>  
>  static inline bool netdev_uses_dsa(struct net_device *dev)
>  {
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 3288a80d4d6c..7a8a0358299b 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -27,6 +27,11 @@
>  
>  #include "dsa_priv.h"
>  
> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
> +{
> +	return !!dst->rcv;
> +}
> +

Hi Vivien

You need to be careful here. This is in the hot path. Every frame
received uses this code. And think about a distro kernel, which might
have DSA enabled by default, yet is unlikely to have any switches. You
are adding a function call which can be called millions of times per
second....

     Andrew

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 30 May 2017 17:01:44 +0200

> On Tue, May 30, 2017 at 10:21:25AM -0400, Vivien Didelot wrote:
>> Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this
>> helper will be extended to access the opaque dsa_device_ops structure.
>> 
>> At the same time, fix the checkpatch comparison check:
>> 
>>     CHECK: Comparison to NULL could be written "dst->rcv"
>>     #41: FILE: net/dsa/dsa.c:32:
>>     +	return dst->rcv != NULL;
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  include/net/dsa.h | 5 +----
>>  net/dsa/dsa.c     | 5 +++++
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index c0e567c0c824..cb5d668b265d 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
>>  
>>  struct net_device *dsa_dev_to_net_device(struct device *dev);
>>  
>> -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>> -{
>> -	return dst->rcv != NULL;
>> -}
>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst);
>>  
>>  static inline bool netdev_uses_dsa(struct net_device *dev)
>>  {
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 3288a80d4d6c..7a8a0358299b 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -27,6 +27,11 @@
>>  
>>  #include "dsa_priv.h"
>>  
>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>> +{
>> +	return !!dst->rcv;
>> +}
>> +
> 
> Hi Vivien
> 
> You need to be careful here. This is in the hot path. Every frame
> received uses this code. And think about a distro kernel, which might
> have DSA enabled by default, yet is unlikely to have any switches. You
> are adding a function call which can be called millions of times per
> second....

Yeah, we really can't make this change.

This isn't glibc where we're trying to hide the implementation of "FILE *"
behind accessor functions that caller can't see.  We inline things when
performance dictates, and it does here.

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[Vivien Didelot]

Hi Andrew, David,

David Miller <davem@davemloft.net> writes:

>>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>>> +{
>>> +	return !!dst->rcv;
>>> +}
>>> +
>> 
>> You need to be careful here. This is in the hot path. Every frame
>> received uses this code. And think about a distro kernel, which might
>> have DSA enabled by default, yet is unlikely to have any switches. You
>> are adding a function call which can be called millions of times per
>> second....
>
> Yeah, we really can't make this change.
>
> This isn't glibc where we're trying to hide the implementation of "FILE *"
> behind accessor functions that caller can't see.  We inline things when
> performance dictates, and it does here.

Thanks for the explanation, this wasn't obvious to me at all. So inline
is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have
an significant impact on performance?

Thanks,

        Vivien

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[Andrew Lunn]

On Tue, May 30, 2017 at 11:56:30AM -0400, Vivien Didelot wrote:
> Hi Andrew, David,
> 
> David Miller <davem@davemloft.net> writes:
> 
> >>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
> >>> +{
> >>> +	return !!dst->rcv;
> >>> +}
> >>> +
> >> 
> >> You need to be careful here. This is in the hot path. Every frame
> >> received uses this code. And think about a distro kernel, which might
> >> have DSA enabled by default, yet is unlikely to have any switches. You
> >> are adding a function call which can be called millions of times per
> >> second....
> >
> > Yeah, we really can't make this change.
> >
> > This isn't glibc where we're trying to hide the implementation of "FILE *"
> > behind accessor functions that caller can't see.  We inline things when
> > performance dictates, and it does here.
> 
> Thanks for the explanation, this wasn't obvious to me at all. So inline
> is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have
> an significant impact on performance?

The additional dereference could cause a cache miss when accessing
tag_ops, which is expensive. dst will be in cache, so dst->rcv should
always be cheap.

       Andrew

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, May 30, 2017 at 11:56:30AM -0400, Vivien Didelot wrote:
>> Hi Andrew, David,
>> 
>> David Miller <davem@davemloft.net> writes:
>> 
>> >>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>> >>> +{
>> >>> +	return !!dst->rcv;
>> >>> +}
>> >>> +
>> >> 
>> >> You need to be careful here. This is in the hot path. Every frame
>> >> received uses this code. And think about a distro kernel, which might
>> >> have DSA enabled by default, yet is unlikely to have any switches. You
>> >> are adding a function call which can be called millions of times per
>> >> second....
>> >
>> > Yeah, we really can't make this change.
>> >
>> > This isn't glibc where we're trying to hide the implementation of "FILE *"
>> > behind accessor functions that caller can't see.  We inline things when
>> > performance dictates, and it does here.
>> 
>> Thanks for the explanation, this wasn't obvious to me at all. So inline
>> is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have
>> an significant impact on performance?
>
> The additional dereference could cause a cache miss when accessing
> tag_ops, which is expensive. dst will be in cache, so dst->rcv should
> always be cheap.

OK! That was interesting. I'm dropping the first 2 patches.

Thanks,

        Vivien

Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

Hi Vivien,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Vivien-Didelot/net-dsa-tagger-simplification/20170531-032911
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   net/built-in.o: In function `eth_type_trans':
>> (.text+0xbf5f2): undefined reference to `dsa_uses_tagged_protocol'
   net/built-in.o: In function `ic_close_devs':
>> ipconfig.c:(.init.text+0x72c2): undefined reference to `dsa_uses_tagged_protocol'
   net/built-in.o: In function `ip_auto_config':
   ipconfig.c:(.init.text+0x8882): undefined reference to `dsa_uses_tagged_protocol'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 47727 bytes --]

[PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA tree

[Vivien Didelot]

The dsa_switch_tree holds a copy of the rcv member of the dsa_device_ops
structure. dst->rcv is always assigned to dst->tag_ops->rcv. Remove this
useless copy.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 include/net/dsa.h | 4 ----
 net/dsa/dsa.c     | 4 ++--
 net/dsa/dsa2.c    | 2 --
 net/dsa/legacy.c  | 2 --
 4 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index cb5d668b265d..4b82766715e9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -126,10 +126,6 @@ struct dsa_switch_tree {
 	 * protocol to use.
 	 */
 	struct net_device	*master_netdev;
-	struct sk_buff *	(*rcv)(struct sk_buff *skb,
-				       struct net_device *dev,
-				       struct packet_type *pt,
-				       struct net_device *orig_dev);
 
 	/*
 	 * Original copy of the master netdev ethtool_ops
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 7a8a0358299b..861dc0e5020d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -29,7 +29,7 @@
 
 bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
 {
-	return !!dst->rcv;
+	return dst->tag_ops && dst->tag_ops->rcv;
 }
 
 static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb,
@@ -209,7 +209,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
-	nskb = dst->rcv(skb, dev, pt, orig_dev);
+	nskb = dst->tag_ops->rcv(skb, dev, pt, orig_dev);
 	if (!nskb) {
 		kfree_skb(skb);
 		return 0;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 4301f52e4f5a..12306c15ed51 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -528,8 +528,6 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 		return PTR_ERR(dst->tag_ops);
 	}
 
-	dst->rcv = dst->tag_ops->rcv;
-
 	return 0;
 }
 
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index ac4379b8d7ac..738c5c2d7d3c 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -150,8 +150,6 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
 		if (IS_ERR(dst->tag_ops))
 			return PTR_ERR(dst->tag_ops);
-
-		dst->rcv = dst->tag_ops->rcv;
 	}
 
 	memcpy(ds->rtable, cd->rtable, sizeof(ds->rtable));
-- 
2.13.0

Re: [PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA tree

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:26AM -0400, Vivien Didelot wrote:
> The dsa_switch_tree holds a copy of the rcv member of the dsa_device_ops
> structure. dst->rcv is always assigned to dst->tag_ops->rcv. Remove this
> useless copy.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  include/net/dsa.h | 4 ----
>  net/dsa/dsa.c     | 4 ++--
>  net/dsa/dsa2.c    | 2 --
>  net/dsa/legacy.c  | 2 --
>  4 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index cb5d668b265d..4b82766715e9 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -126,10 +126,6 @@ struct dsa_switch_tree {
>  	 * protocol to use.
>  	 */
>  	struct net_device	*master_netdev;
> -	struct sk_buff *	(*rcv)(struct sk_buff *skb,
> -				       struct net_device *dev,
> -				       struct packet_type *pt,
> -				       struct net_device *orig_dev);
>  
>  	/*
>  	 * Original copy of the master netdev ethtool_ops
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 7a8a0358299b..861dc0e5020d 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -29,7 +29,7 @@
>  
>  bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>  {
> -	return !!dst->rcv;
> +	return dst->tag_ops && dst->tag_ops->rcv;

This makes the hot path more expensive. The copy is probably worth it
in terms of performance.

   Andrew

Re: [PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA tree

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 30 May 2017 17:05:31 +0200

> On Tue, May 30, 2017 at 10:21:26AM -0400, Vivien Didelot wrote:
>> The dsa_switch_tree holds a copy of the rcv member of the dsa_device_ops
>> structure. dst->rcv is always assigned to dst->tag_ops->rcv. Remove this
>> useless copy.
>> 
>> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>> ---
>>  include/net/dsa.h | 4 ----
>>  net/dsa/dsa.c     | 4 ++--
>>  net/dsa/dsa2.c    | 2 --
>>  net/dsa/legacy.c  | 2 --
>>  4 files changed, 2 insertions(+), 10 deletions(-)
>> 
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index cb5d668b265d..4b82766715e9 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -126,10 +126,6 @@ struct dsa_switch_tree {
>>  	 * protocol to use.
>>  	 */
>>  	struct net_device	*master_netdev;
>> -	struct sk_buff *	(*rcv)(struct sk_buff *skb,
>> -				       struct net_device *dev,
>> -				       struct packet_type *pt,
>> -				       struct net_device *orig_dev);
>>  
>>  	/*
>>  	 * Original copy of the master netdev ethtool_ops
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 7a8a0358299b..861dc0e5020d 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -29,7 +29,7 @@
>>  
>>  bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
>>  {
>> -	return !!dst->rcv;
>> +	return dst->tag_ops && dst->tag_ops->rcv;
> 
> This makes the hot path more expensive. The copy is probably worth it
> in terms of performance.

Agreed.

[PATCH net-next 3/7] net: dsa: remove unused arguments of tagger rcv

[Vivien Didelot]

The struct dsa_device_ops defines the rcv function with 2 unused
arguments struct packet_type *pt, and struct net_device *orig_dev.

This patch removes them from the definition and implementations.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c         | 2 +-
 net/dsa/dsa_priv.h    | 4 +---
 net/dsa/tag_brcm.c    | 4 +---
 net/dsa/tag_dsa.c     | 4 +---
 net/dsa/tag_edsa.c    | 4 +---
 net/dsa/tag_lan9303.c | 3 +--
 net/dsa/tag_mtk.c     | 4 +---
 net/dsa/tag_qca.c     | 4 +---
 net/dsa/tag_trailer.c | 4 +---
 9 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 861dc0e5020d..98173a3f6fd1 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -209,7 +209,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
-	nskb = dst->tag_ops->rcv(skb, dev, pt, orig_dev);
+	nskb = dst->tag_ops->rcv(skb, dev);
 	if (!nskb) {
 		kfree_skb(skb);
 		return 0;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index c1d4180651af..0fdd2a8a4ad8 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -67,9 +67,7 @@ struct dsa_notifier_vlan_info {
 
 struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
-	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt,
-			       struct net_device *orig_dev);
+	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
 };
 
 struct dsa_slave_priv {
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 9f204f18ada3..c8bff4d7543d 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -92,9 +92,7 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
 	return NULL;
 }
 
-static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				    struct packet_type *pt,
-				    struct net_device *orig_dev)
+static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 3b62a57956a3..41f78e42f9ba 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -68,9 +68,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NULL;
 }
 
-static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt,
-			       struct net_device *orig_dev)
+static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index f95cafd05702..36389fc8fe06 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -81,9 +81,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NULL;
 }
 
-static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
-				struct packet_type *pt,
-				struct net_device *orig_dev)
+static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index afd59330b5f1..82e150486497 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -71,8 +71,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NULL;
 }
 
-static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev,
-			struct packet_type *pt, struct net_device *orig_dev)
+static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	u16 *lan9303_tag;
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index d1258e84cd71..7c9764c8d61b 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -47,9 +47,7 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	return NULL;
 }
 
-static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt,
-				   struct net_device *orig_dev)
+static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 2451007699b7..d38bd5cbfad8 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -66,9 +66,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NULL;
 }
 
-static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt,
-				   struct net_device *orig_dev)
+static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 7488ab2932ab..e1fd78197fc8 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -58,9 +58,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
 	return nskb;
 }
 
-static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
-				   struct packet_type *pt,
-				   struct net_device *orig_dev)
+static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
 	struct dsa_switch *ds;
-- 
2.13.0

Re: [PATCH net-next 3/7] net: dsa: remove unused arguments of tagger rcv

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:27AM -0400, Vivien Didelot wrote:
> The struct dsa_device_ops defines the rcv function with 2 unused
> arguments struct packet_type *pt, and struct net_device *orig_dev.
> 
> This patch removes them from the definition and implementations.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

[Vivien Didelot]

If the receive function of a tagger reallocated the SKB, the original
SKB is currently not freed. Fix this and free it on both copy or error.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 98173a3f6fd1..0b6f2c7d7d1c 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -209,11 +209,12 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
+	/* Receive function may have to reallocate the original SKB */
 	nskb = dst->tag_ops->rcv(skb, dev);
-	if (!nskb) {
+	if (nskb != skb)
 		kfree_skb(skb);
+	if (!nskb)
 		return 0;
-	}
 
 	skb = nskb;
 	skb_push(skb, ETH_HLEN);
-- 
2.13.0

Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
> If the receive function of a tagger reallocated the SKB, the original
> SKB is currently not freed. Fix this and free it on both copy or error.

I don't see any of the receive functions reallocate the skb. It might
be better to just simplify the code to take away the option to return
a different skb.

  Andrew

Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
>> If the receive function of a tagger reallocated the SKB, the original
>> SKB is currently not freed. Fix this and free it on both copy or error.
>
> I don't see any of the receive functions reallocate the skb. It might
> be better to just simplify the code to take away the option to return
> a different skb.

I think it was written such way to be symmetrical with the xmit
implementation, where the trailer tagger does reallocate the skb.

I would say that keeping the symmetry is simpler from the point of view
of the tagging implementations, but I don't have strong opinion here.

Thanks,

        Vivien

Re: [PATCH net-next 4/7] net: dsa: free orig skb on rcv if reallocated

[Andrew Lunn]

On Tue, May 30, 2017 at 11:41:51AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > On Tue, May 30, 2017 at 10:21:28AM -0400, Vivien Didelot wrote:
> >> If the receive function of a tagger reallocated the SKB, the original
> >> SKB is currently not freed. Fix this and free it on both copy or error.
> >
> > I don't see any of the receive functions reallocate the skb. It might
> > be better to just simplify the code to take away the option to return
> > a different skb.
> 
> I think it was written such way to be symmetrical with the xmit
> implementation, where the trailer tagger does reallocate the skb.

trailer_xmit() releases the original and so does the in-review KSZ
tagger.

So i think in general, Rx and Rx, the tagger should be responsible for
freeing the original, but on error the core should do the free.

	Andrew

[PATCH net-next 5/7] net: dsa: remove useless goto label in tagger rcv

[Vivien Didelot]

Many rcv functions from net/dsa/tag_*.c have a useless out_drop goto
label which simply returns NULL. Kill it in favor of the obvious.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/tag_brcm.c    | 11 ++++-------
 net/dsa/tag_dsa.c     | 13 +++++--------
 net/dsa/tag_edsa.c    | 13 +++++--------
 net/dsa/tag_mtk.c     |  9 +++------
 net/dsa/tag_qca.c     | 11 ++++-------
 net/dsa/tag_trailer.c |  9 +++------
 6 files changed, 24 insertions(+), 42 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index c8bff4d7543d..10fa4c0ca46b 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -102,27 +102,27 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	ds = dst->cpu_dp->ds;
 
 	if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
-		goto out_drop;
+		return NULL;
 
 	/* skb->data points to the EtherType, the tag is right before it */
 	brcm_tag = skb->data - 2;
 
 	/* The opcode should never be different than 0b000 */
 	if (unlikely((brcm_tag[0] >> BRCM_OPCODE_SHIFT) & BRCM_OPCODE_MASK))
-		goto out_drop;
+		return NULL;
 
 	/* We should never see a reserved reason code without knowing how to
 	 * handle it
 	 */
 	if (unlikely(brcm_tag[2] & BRCM_EG_RC_RSVD))
-		goto out_drop;
+		return NULL;
 
 	/* Locate which port this is coming from */
 	source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
 
 	/* Validate port against switch setup, either the port is totally */
 	if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
-		goto out_drop;
+		return NULL;
 
 	/* Remove Broadcom tag and update checksum */
 	skb_pull_rcsum(skb, BRCM_TAG_LEN);
@@ -135,9 +135,6 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	skb->dev = ds->ports[source_port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops brcm_netdev_ops = {
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 41f78e42f9ba..9f5fecaa4a93 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -77,7 +77,7 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	int source_port;
 
 	if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * The ethertype field is part of the DSA header.
@@ -88,7 +88,7 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	 * Check that frame type is either TO_CPU or FORWARD.
 	 */
 	if ((dsa_header[0] & 0xc0) != 0x00 && (dsa_header[0] & 0xc0) != 0xc0)
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * Determine source device and port.
@@ -101,14 +101,14 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	 * port is a registered DSA port.
 	 */
 	if (source_device >= DSA_MAX_SWITCHES)
-		goto out_drop;
+		return NULL;
 
 	ds = dst->ds[source_device];
 	if (!ds)
-		goto out_drop;
+		return NULL;
 
 	if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * Convert the DSA header to an 802.1q header if the 'tagged'
@@ -159,9 +159,6 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	skb->dev = ds->ports[source_port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops dsa_netdev_ops = {
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 36389fc8fe06..a9a06e19abeb 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -90,7 +90,7 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	int source_port;
 
 	if (unlikely(!pskb_may_pull(skb, EDSA_HLEN)))
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * Skip the two null bytes after the ethertype.
@@ -101,7 +101,7 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	 * Check that frame type is either TO_CPU or FORWARD.
 	 */
 	if ((edsa_header[0] & 0xc0) != 0x00 && (edsa_header[0] & 0xc0) != 0xc0)
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * Determine source device and port.
@@ -114,14 +114,14 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	 * port is a registered DSA port.
 	 */
 	if (source_device >= DSA_MAX_SWITCHES)
-		goto out_drop;
+		return NULL;
 
 	ds = dst->ds[source_device];
 	if (!ds)
-		goto out_drop;
+		return NULL;
 
 	if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
-		goto out_drop;
+		return NULL;
 
 	/*
 	 * If the 'tagged' bit is set, convert the DSA tag to a 802.1q
@@ -178,9 +178,6 @@ static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
 	skb->dev = ds->ports[source_port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops edsa_netdev_ops = {
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 7c9764c8d61b..327dd7b596df 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -55,7 +55,7 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	__be16 *phdr, hdr;
 
 	if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
-		goto out_drop;
+		return NULL;
 
 	/* The MTK header is added by the switch between src addr
 	 * and ethertype at this point, skb->data points to 2 bytes
@@ -77,19 +77,16 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	 */
 	ds = dst->ds[0];
 	if (!ds)
-		goto out_drop;
+		return NULL;
 
 	/* Get source port information */
 	port = (hdr & MTK_HDR_RECV_SOURCE_PORT_MASK);
 	if (!ds->ports[port].netdev)
-		goto out_drop;
+		return NULL;
 
 	skb->dev = ds->ports[port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops mtk_netdev_ops = {
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index d38bd5cbfad8..8080ad8f2abb 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -75,7 +75,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	__be16 *phdr, hdr;
 
 	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
-		goto out_drop;
+		return NULL;
 
 	/* The QCA header is added by the switch between src addr and Ethertype
 	 * At this point, skb->data points to ethertype so header should be
@@ -87,7 +87,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	/* Make sure the version is correct */
 	ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
 	if (unlikely(ver != QCA_HDR_VERSION))
-		goto out_drop;
+		return NULL;
 
 	/* Remove QCA tag and recalculate checksum */
 	skb_pull_rcsum(skb, QCA_HDR_LEN);
@@ -99,20 +99,17 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	 */
 	ds = dst->cpu_dp->ds;
 	if (!ds)
-		goto out_drop;
+		return NULL;
 
 	/* Get source port information */
 	port = (hdr & QCA_HDR_RECV_SOURCE_PORT_MASK);
 	if (!ds->ports[port].netdev)
-		goto out_drop;
+		return NULL;
 
 	/* Update skb & forward the frame accordingly */
 	skb->dev = ds->ports[port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops qca_netdev_ops = {
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index e1fd78197fc8..7422b1329712 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -68,25 +68,22 @@ static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev)
 	ds = dst->cpu_dp->ds;
 
 	if (skb_linearize(skb))
-		goto out_drop;
+		return NULL;
 
 	trailer = skb_tail_pointer(skb) - 4;
 	if (trailer[0] != 0x80 || (trailer[1] & 0xf8) != 0x00 ||
 	    (trailer[2] & 0xef) != 0x00 || trailer[3] != 0x00)
-		goto out_drop;
+		return NULL;
 
 	source_port = trailer[1] & 7;
 	if (source_port >= ds->num_ports || !ds->ports[source_port].netdev)
-		goto out_drop;
+		return NULL;
 
 	pskb_trim_rcsum(skb, skb->len - 4);
 
 	skb->dev = ds->ports[source_port].netdev;
 
 	return skb;
-
-out_drop:
-	return NULL;
 }
 
 const struct dsa_device_ops trailer_netdev_ops = {
-- 
2.13.0

[PATCH net-next 6/7] net: dsa: remove useless copy of tagger xmit

[Vivien Didelot]

The dsa_slave_priv structure holds a copy of the dsa_device_ops xmit
function. It is always assigned to the switch tree xmit function.

Remove this useless copy.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/dsa_priv.h | 3 ---
 net/dsa/slave.c    | 4 +---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 0fdd2a8a4ad8..282a55639285 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -71,9 +71,6 @@ struct dsa_device_ops {
 };
 
 struct dsa_slave_priv {
-	struct sk_buff *	(*xmit)(struct sk_buff *skb,
-					struct net_device *dev);
-
 	/* DSA port data, such as switch, port index, etc. */
 	struct dsa_port		*dp;
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 887e26695519..ebad2af4d3a8 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -363,7 +363,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.tx_bytes += skb->len;
 
 	/* Transmit function may have to reallocate the original SKB */
-	nskb = p->xmit(skb, dev);
+	nskb = p->dp->ds->dst->tag_ops->xmit(skb, dev);
 	if (!nskb)
 		return NETDEV_TX_OK;
 
@@ -1136,7 +1136,6 @@ int dsa_slave_resume(struct net_device *slave_dev)
 int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		     int port, const char *name)
 {
-	struct dsa_switch_tree *dst = ds->dst;
 	struct net_device *master;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
@@ -1172,7 +1171,6 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	p = netdev_priv(slave_dev);
 	p->dp = &ds->ports[port];
 	INIT_LIST_HEAD(&p->mall_tc_list);
-	p->xmit = dst->tag_ops->xmit;
 
 	p->old_pause = -1;
 	p->old_link = -1;
-- 
2.13.0

Re: [PATCH net-next 6/7] net: dsa: remove useless copy of tagger xmit

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:30AM -0400, Vivien Didelot wrote:
> The dsa_slave_priv structure holds a copy of the dsa_device_ops xmit
> function. It is always assigned to the switch tree xmit function.
> 
> Remove this useless copy.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>  net/dsa/dsa_priv.h | 3 ---
>  net/dsa/slave.c    | 4 +---
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 0fdd2a8a4ad8..282a55639285 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -71,9 +71,6 @@ struct dsa_device_ops {
>  };
>  
>  struct dsa_slave_priv {
> -	struct sk_buff *	(*xmit)(struct sk_buff *skb,
> -					struct net_device *dev);
> -
>  	/* DSA port data, such as switch, port index, etc. */
>  	struct dsa_port		*dp;
>  
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 887e26695519..ebad2af4d3a8 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -363,7 +363,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
>  	dev->stats.tx_bytes += skb->len;
>  
>  	/* Transmit function may have to reallocate the original SKB */
> -	nskb = p->xmit(skb, dev);
> +	nskb = p->dp->ds->dst->tag_ops->xmit(skb, dev);

This is also the hot path for DSA transmit. Do we really want to do 4
extra pointer dereferences a million times per second, compared to one
copy during setup?

     Andrew

Re: [PATCH net-next 6/7] net: dsa: remove useless copy of tagger xmit

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>>  	/* Transmit function may have to reallocate the original SKB */
>> -	nskb = p->xmit(skb, dev);
>> +	nskb = p->dp->ds->dst->tag_ops->xmit(skb, dev);
>
> This is also the hot path for DSA transmit. Do we really want to do 4
> extra pointer dereferences a million times per second, compared to one
> copy during setup?

Yep I get the idea. It felt weird to copy structure members like this
and not at least reusing the dsa_device_ops structure.

Thanks,

        Vivien

[PATCH net-next 7/7] net: dsa: factor skb freeing on xmit

[Vivien Didelot]

The taggers are currently responsible to free the original SKB if they
made a copy of it, or in case of error.

This patch simplifies this by freeing the original SKB in the
dsa_slave_xmit caller if it differs from the return SKB (copy or NULL.)

At the same time, fix the checkpatch NULL comparison check:

        CHECK: Comparison to NULL could be written "!nskb"
    #208: FILE: net/dsa/tag_trailer.c:35:
    +	if (nskb == NULL)

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c       | 2 ++
 net/dsa/tag_brcm.c    | 6 +-----
 net/dsa/tag_dsa.c     | 8 ++------
 net/dsa/tag_edsa.c    | 8 ++------
 net/dsa/tag_lan9303.c | 5 +----
 net/dsa/tag_mtk.c     | 6 +-----
 net/dsa/tag_qca.c     | 6 +-----
 net/dsa/tag_trailer.c | 5 +----
 8 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ebad2af4d3a8..a520c822436e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -364,6 +364,8 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Transmit function may have to reallocate the original SKB */
 	nskb = p->dp->ds->dst->tag_ops->xmit(skb, dev);
+	if (nskb != skb)
+		kfree_skb(skb);
 	if (!nskb)
 		return NETDEV_TX_OK;
 
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 10fa4c0ca46b..5db6bcfde025 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -65,7 +65,7 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
 	u8 *brcm_tag;
 
 	if (skb_cow_head(skb, BRCM_TAG_LEN) < 0)
-		goto out_free;
+		return NULL;
 
 	skb_push(skb, BRCM_TAG_LEN);
 
@@ -86,10 +86,6 @@ static struct sk_buff *brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev
 	brcm_tag[3] = (1 << p->dp->index) & BRCM_IG_DSTMAP1_MASK;
 
 	return skb;
-
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 9f5fecaa4a93..6837ca9160a8 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -28,7 +28,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	if (skb->protocol == htons(ETH_P_8021Q)) {
 		if (skb_cow_head(skb, 0) < 0)
-			goto out_free;
+			return NULL;
 
 		/*
 		 * Construct tagged FROM_CPU DSA tag from 802.1q tag.
@@ -46,7 +46,7 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	} else {
 		if (skb_cow_head(skb, DSA_HLEN) < 0)
-			goto out_free;
+			return NULL;
 		skb_push(skb, DSA_HLEN);
 
 		memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
@@ -62,10 +62,6 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	return skb;
-
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index a9a06e19abeb..96ddc90472a2 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -30,7 +30,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	 */
 	if (skb->protocol == htons(ETH_P_8021Q)) {
 		if (skb_cow_head(skb, DSA_HLEN) < 0)
-			goto out_free;
+			return NULL;
 		skb_push(skb, DSA_HLEN);
 
 		memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
@@ -55,7 +55,7 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	} else {
 		if (skb_cow_head(skb, EDSA_HLEN) < 0)
-			goto out_free;
+			return NULL;
 		skb_push(skb, EDSA_HLEN);
 
 		memmove(skb->data, skb->data + EDSA_HLEN, 2 * ETH_ALEN);
@@ -75,10 +75,6 @@ static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	return skb;
-
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_lan9303.c b/net/dsa/tag_lan9303.c
index 82e150486497..e33348101e8f 100644
--- a/net/dsa/tag_lan9303.c
+++ b/net/dsa/tag_lan9303.c
@@ -52,7 +52,7 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (skb_cow_head(skb, LAN9303_TAG_LEN) < 0) {
 		dev_dbg(&dev->dev,
 			"Cannot make room for the special tag. Dropping packet\n");
-		goto out_free;
+		return NULL;
 	}
 
 	/* provide 'LAN9303_TAG_LEN' bytes additional space */
@@ -66,9 +66,6 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device *dev)
 	lan9303_tag[1] = htons(p->dp->index | BIT(4));
 
 	return skb;
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *lan9303_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 327dd7b596df..ee62ae5f03e1 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -27,7 +27,7 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	u8 *mtk_tag;
 
 	if (skb_cow_head(skb, MTK_HDR_LEN) < 0)
-		goto out_free;
+		return NULL;
 
 	skb_push(skb, MTK_HDR_LEN);
 
@@ -41,10 +41,6 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	mtk_tag[3] = 0;
 
 	return skb;
-
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 8080ad8f2abb..4d3bc2920477 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -45,7 +45,7 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.tx_bytes += skb->len;
 
 	if (skb_cow_head(skb, 0) < 0)
-		goto out_free;
+		return NULL;
 
 	skb_push(skb, QCA_HDR_LEN);
 
@@ -60,10 +60,6 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	*phdr = htons(hdr);
 
 	return skb;
-
-out_free:
-	kfree_skb(skb);
-	return NULL;
 }
 
 static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 7422b1329712..05dafd7cdb21 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -32,17 +32,14 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb, struct net_device *dev)
 		padlen = 60 - skb->len;
 
 	nskb = alloc_skb(NET_IP_ALIGN + skb->len + padlen + 4, GFP_ATOMIC);
-	if (nskb == NULL) {
-		kfree_skb(skb);
+	if (!nskb)
 		return NULL;
-	}
 	skb_reserve(nskb, NET_IP_ALIGN);
 
 	skb_reset_mac_header(nskb);
 	skb_set_network_header(nskb, skb_network_header(skb) - skb->head);
 	skb_set_transport_header(nskb, skb_transport_header(skb) - skb->head);
 	skb_copy_and_csum_dev(skb, skb_put(nskb, skb->len));
-	kfree_skb(skb);
 
 	if (padlen) {
 		u8 *pad = skb_put(nskb, padlen);
-- 
2.13.0

Re: [PATCH net-next 7/7] net: dsa: factor skb freeing on xmit

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:31AM -0400, Vivien Didelot wrote:
> The taggers are currently responsible to free the original SKB if they
> made a copy of it, or in case of error.
> 
> This patch simplifies this by freeing the original SKB in the
> dsa_slave_xmit caller if it differs from the return SKB (copy or NULL.)

So we have two different things here:

1) The tagger needs to drop the frame, because there is an error of
some sort.

2) The tagger allocates a new skb for the frame.

I think on error, the core code can drop the frame.

If the tagger allocates a new skb, i think the tagger should be
responsible for freeing to original.

	    Andrew

Re: [PATCH net-next 0/7] net: dsa: tagger simplification

[Andrew Lunn]

On Tue, May 30, 2017 at 10:21:24AM -0400, Vivien Didelot wrote:
> The DSA layer has a dsa_device_ops structure containing two members to
> tag and untag the proprietary switch header, called xmit and rcv.
> 
> The switch tree and slave private structures respectively hold a useless
> copy of the rcv and xmit functions. The tagging implementations use
> useless goto labels and only the rcv caller (partially) handles the SKB
> freeing. The rcv function also contains unused arguments.
> 
> This patchset removes the copy of rcv and xmit functions, the unused
> arguments of the rcv signature and the useless labels, and handles
> freeing of the SKB in the xmit caller.

Hi Vivien

Since these changes are dealing with the hot path of the network
stack, it would be good to see some performance results. In
particular, from a high performance machine, 10Gbps interfaces, where
DSA is enabled, as a distro kernel might have.

    Andrew