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

Groups > linux.kernel > #1707323 > unrolled thread

[PATCH 0/4] net-next: dsa: fix flow dissection

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] net-next: dsa: fix flow dissection John Crispin <john@phrozen.org> - 2017-08-09 14:50 +0200
    [PATCH 3/4] net-next: tag_mtk: add flow_dissect callback to the ops struct John Crispin <john@phrozen.org> - 2017-08-09 14:50 +0200
      Re: [PATCH 3/4] net-next: tag_mtk: add flow_dissect callback to the  ops struct Andrew Lunn <andrew@lunn.ch> - 2017-08-09 16:00 +0200
    Re: [PATCH 0/4] net-next: dsa: fix flow dissection Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-08-09 17:20 +0200
    Re: [PATCH 0/4] net-next: dsa: fix flow dissection David Miller <davem@davemloft.net> - 2017-08-10 08:00 +0200
      Re: [PATCH 0/4] net-next: dsa: fix flow dissection Eric Dumazet <eric.dumazet@gmail.com> - 2017-08-10 08:50 +0200
        Re: [PATCH 0/4] net-next: dsa: fix flow dissection John Crispin <john@phrozen.org> - 2017-08-10 09:50 +0200
        Re: [PATCH 0/4] net-next: dsa: fix flow dissection John Crispin <john@phrozen.org> - 2017-08-10 10:20 +0200

#1707323 — [PATCH 0/4] net-next: dsa: fix flow dissection

RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this is that skb_hash will call the
flow_dissector. At this point the skb still contains the magic switch
header and the skb->protocol field is not set up to the correct 802.3
value yet. By the time the tag specific code is called, removing the header
and properly setting the protocol an invalid hash is already set. In the
case of the mt7530 this will result in all flows always having the same
hash.

Changes since RFC:
* use a callback instead of static values
* add cover letter

John Crispin (4):
  net-next: dsa: move struct dsa_device_ops to the global header file
  net-next: dsa: add flow_dissect callback to struct dsa_device_ops
  net-next: tag_mtk: add flow_dissect callback to the ops struct
  net-next: dsa: fix flow dissection

 include/net/dsa.h         |  9 +++++++++
 net/core/flow_dissector.c | 12 ++++++++++++
 net/dsa/dsa_priv.h        |  7 -------
 net/dsa/tag_mtk.c         | 14 ++++++++++++--
 4 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1707325 — [PATCH 3/4] net-next: tag_mtk: add flow_dissect callback to the ops struct

The MT7530 inserts the 4 magic header in between the 802.3 address and
protocol field. The patch implements the callback that can be called by
the flow dissector to figure out the real protocol and offset of the
network header. With this patch applied we can properly parse the packet
and thus make hashing function properly.

Signed-off-by: Muciri Gatimu <muciri@openmesh.com>
Signed-off-by: Shashidhar Lakkavalli <shashidhar.lakkavalli@openmesh.com>
Signed-off-by: John Crispin <john@phrozen.org>
---
 net/dsa/tag_mtk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index 2f32b7ea3365..02163c045a96 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -87,7 +87,17 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 }
 
+static int mtk_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+				int *offset)
+{
+	*offset = 4;
+	*proto = ((__be16 *)skb->data)[1];
+
+	return 0;
+}
+
 const struct dsa_device_ops mtk_netdev_ops = {
-	.xmit	= mtk_tag_xmit,
-	.rcv	= mtk_tag_rcv,
+	.xmit		= mtk_tag_xmit,
+	.rcv		= mtk_tag_rcv,
+	.flow_dissect	= mtk_tag_flow_dissect,
 };
-- 
2.11.0

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

#1707356 — Re: [PATCH 3/4] net-next: tag_mtk: add flow_dissect callback to the ops struct

On Wed, Aug 09, 2017 at 02:41:18PM +0200, John Crispin wrote:
> The MT7530 inserts the 4 magic header in between the 802.3 address and
> protocol field. The patch implements the callback that can be called by
> the flow dissector to figure out the real protocol and offset of the
> network header. With this patch applied we can properly parse the packet
> and thus make hashing function properly.
> 
> Signed-off-by: Muciri Gatimu <muciri@openmesh.com>
> Signed-off-by: Shashidhar Lakkavalli <shashidhar.lakkavalli@openmesh.com>
> Signed-off-by: John Crispin <john@phrozen.org>

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

    Andrew

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

#1707411

Hi John,

John Crispin <john@phrozen.org> writes:

> RPS and probably other kernel features are currently broken on some if not
> all DSA devices. The root cause of this is that skb_hash will call the
> flow_dissector. At this point the skb still contains the magic switch
> header and the skb->protocol field is not set up to the correct 802.3
> value yet. By the time the tag specific code is called, removing the header
> and properly setting the protocol an invalid hash is already set. In the
> case of the mt7530 this will result in all flows always having the same
> hash.
>
> Changes since RFC:
> * use a callback instead of static values
> * add cover letter
>
> John Crispin (4):
>   net-next: dsa: move struct dsa_device_ops to the global header file
>   net-next: dsa: add flow_dissect callback to struct dsa_device_ops
>   net-next: tag_mtk: add flow_dissect callback to the ops struct
>   net-next: dsa: fix flow dissection

The "net-next" tag goes into the subject-prefix, i.e.
"[PATCH net-next v3 0/4]" (this is the result of git format-patch
--subject-prefix="PATCH net-next" -v3 --cover-letter.)

The commit title prefix represents the most impacted subsystem, here
"net: dsa: xxx" would map the net/dsa/ or drivers/net/dsa directories.

Other than that, the whole patchset LGTM:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>


Thanks,

        Vivien

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

#1708230

From: John Crispin <john@phrozen.org>
Date: Wed,  9 Aug 2017 14:41:15 +0200

> RPS and probably other kernel features are currently broken on some if not
> all DSA devices. The root cause of this is that skb_hash will call the
> flow_dissector. At this point the skb still contains the magic switch
> header and the skb->protocol field is not set up to the correct 802.3
> value yet. By the time the tag specific code is called, removing the header
> and properly setting the protocol an invalid hash is already set. In the
> case of the mt7530 this will result in all flows always having the same
> hash.
> 
> Changes since RFC:
> * use a callback instead of static values
> * add cover letter

Series applied, thanks.

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

#1708257

On Wed, 2017-08-09 at 22:52 -0700, David Miller wrote:
> From: John Crispin <john@phrozen.org>
> Date: Wed,  9 Aug 2017 14:41:15 +0200
> 
> > RPS and probably other kernel features are currently broken on some if not
> > all DSA devices. The root cause of this is that skb_hash will call the
> > flow_dissector. At this point the skb still contains the magic switch
> > header and the skb->protocol field is not set up to the correct 802.3
> > value yet. By the time the tag specific code is called, removing the header
> > and properly setting the protocol an invalid hash is already set. In the
> > case of the mt7530 this will result in all flows always having the same
> > hash.
> > 
> > Changes since RFC:
> > * use a callback instead of static values
> > * add cover letter
> 
> Series applied, thanks.

Is this related ?

net/core/flow_dissector.c: In function '__skb_flow_dissect':
net/core/flow_dissector.c:448:18: error: 'struct net_device' has no member named 'dsa_ptr'
    ops = skb->dev->dsa_ptr->tag_ops;
                  ^
make[3]: *** [net/core/flow_dissector.o] Error 1

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

#1708284

On 10/08/17 08:42, Eric Dumazet wrote:
> On Wed, 2017-08-09 at 22:52 -0700, David Miller wrote:
>> From: John Crispin <john@phrozen.org>
>> Date: Wed,  9 Aug 2017 14:41:15 +0200
>>
>>> RPS and probably other kernel features are currently broken on some if not
>>> all DSA devices. The root cause of this is that skb_hash will call the
>>> flow_dissector. At this point the skb still contains the magic switch
>>> header and the skb->protocol field is not set up to the correct 802.3
>>> value yet. By the time the tag specific code is called, removing the header
>>> and properly setting the protocol an invalid hash is already set. In the
>>> case of the mt7530 this will result in all flows always having the same
>>> hash.
>>>
>>> Changes since RFC:
>>> * use a callback instead of static values
>>> * add cover letter
>> Series applied, thanks.
> Is this related ?
>
> net/core/flow_dissector.c: In function '__skb_flow_dissect':
> net/core/flow_dissector.c:448:18: error: 'struct net_device' has no member named 'dsa_ptr'
>      ops = skb->dev->dsa_ptr->tag_ops;
>                    ^
> make[3]: *** [net/core/flow_dissector.o] Error 1
>
>

looks like it, I did test the patches against net-next from 24 hours 
ago, let me do a test build just now.
     John


> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

#1708303

On 10/08/17 08:42, Eric Dumazet wrote:
> On Wed, 2017-08-09 at 22:52 -0700, David Miller wrote:
>> From: John Crispin <john@phrozen.org>
>> Date: Wed,  9 Aug 2017 14:41:15 +0200
>>
>>> RPS and probably other kernel features are currently broken on some if not
>>> all DSA devices. The root cause of this is that skb_hash will call the
>>> flow_dissector. At this point the skb still contains the magic switch
>>> header and the skb->protocol field is not set up to the correct 802.3
>>> value yet. By the time the tag specific code is called, removing the header
>>> and properly setting the protocol an invalid hash is already set. In the
>>> case of the mt7530 this will result in all flows always having the same
>>> hash.
>>>
>>> Changes since RFC:
>>> * use a callback instead of static values
>>> * add cover letter
>> Series applied, thanks.
> Is this related ?
>
> net/core/flow_dissector.c: In function '__skb_flow_dissect':
> net/core/flow_dissector.c:448:18: error: 'struct net_device' has no member named 'dsa_ptr'
>      ops = skb->dev->dsa_ptr->tag_ops;
>                    ^
> make[3]: *** [net/core/flow_dissector.o] Error 1
>
>
Hi Eric,

I have just sent the fix for this compile error

     John

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web