Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653273 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/7] net: dsa: tagger simplification Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
    [PATCH net-next 3/7] net: dsa: remove unused arguments of tagger rcv Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
      Re: [PATCH net-next 3/7] net: dsa: remove unused arguments of tagger  rcv Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:20 +0200
    [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
      Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:10 +0200
        Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol  code David Miller <davem@davemloft.net> - 2017-05-30 17:20 +0200
          Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 18:00 +0200
            Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 18:20 +0200
            Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code Andrew Lunn <andrew@lunn.ch> - 2017-05-30 18:20 +0200
    [PATCH net-next 7/7] net: dsa: factor skb freeing on xmit Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
      Re: [PATCH net-next 7/7] net: dsa: factor skb freeing on xmit Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:40 +0200
    [PATCH net-next 5/7] net: dsa: remove useless goto label in tagger rcv Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
    [PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA tree Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-05-30 16:30 +0200
      Re: [PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA  tree Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:10 +0200
        Re: [PATCH net-next 2/7] net: dsa: remove useless rcv copy in DSA  tree David Miller <davem@davemloft.net> - 2017-05-30 17:20 +0200
    Re: [PATCH net-next 0/7] net: dsa: tagger simplification Andrew Lunn <andrew@lunn.ch> - 2017-05-30 17:20 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1653308

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web