From: John Crispin <john@phrozen.org> Date: 2017-06-20 08:07:22
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector. 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.
The patch adds 2 new fields to the dsa_switch_ops allowing the
flow_disector to use them in order to be able to create the real hash of
the connection.
Signed-off-by: John Crispin <john@phrozen.org>
---
include/net/dsa.h | 6 ++++++
net/core/flow_dissector.c | 12 ++++++++++++
2 files changed, 18 insertions(+)
@@ -440,6 +441,17 @@ bool __skb_flow_dissect(const struct sk_buff *skb,skb->vlan_proto:skb->protocol;nhoff=skb_network_offset(skb);hlen=skb_headlen(skb);++if(unlikely(netdev_uses_dsa(skb->dev))){+conststructdsa_switch_ops*ops;+u8*p=(u8*)data;++ops=skb->dev->dsa_ptr->ds[0]->ops;+if(ops->hash_proto_off)+proto=(u16)p[ops->hash_proto_off];+hlen-=ops->hash_nh_off;+nhoff+=ops->hash_nh_off;+}}/* It is ensured by skb_flow_dissector_init() that control key will
From: John Crispin <john@phrozen.org> Date: 2017-06-20 08:07:21
The MT7530 inserts the 4 magic header in between the 802.3 address and
protocol field. The patch defines these header such that the flow_disector
can properly parse the packet and thus allows hashing to function properly.
Signed-off-by: John Crispin <john@phrozen.org>
---
drivers/net/dsa/mt7530.c | 2 ++
1 file changed, 2 insertions(+)
From: Sergei Shtylyov <hidden> Date: 2017-06-20 10:17:41
Hello!
On 6/20/2017 11:06 AM, John Crispin wrote:
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
"Is" missing between "this" and "that"?
flow_disector. At this point the skb still contains the magic switch header
Dissector?
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.
The patch adds 2 new fields to the dsa_switch_ops allowing the
flow_disector to use them in order to be able to create the real hash of
Again.
the connection.
Signed-off-by: John Crispin <john@phrozen.org>
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-06-20 13:54:59
On Tue, Jun 20, 2017 at 10:06:55AM +0200, John Crispin wrote:
The MT7530 inserts the 4 magic header in between the 802.3 address and
protocol field. The patch defines these header such that the flow_disector
can properly parse the packet and thus allows hashing to function properly.
This is to do with tagging, not the switch driver. The Marvell switch
driver can be used with two different tagging protocols.
So i would put these fields in the dsa_device_ops.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-06-20 14:01:50
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Andrew
From: John Crispin <john@phrozen.org> Date: 2017-06-20 17:27:55
On 20/06/17 15:54, Andrew Lunn wrote:
On Tue, Jun 20, 2017 at 10:06:55AM +0200, John Crispin wrote:
quoted
The MT7530 inserts the 4 magic header in between the 802.3 address and
protocol field. The patch defines these header such that the flow_disector
can properly parse the packet and thus allows hashing to function properly.
This is to do with tagging, not the switch driver. The Marvell switch
driver can be used with two different tagging protocols.
So i would put these fields in the dsa_device_ops.
Andrew
Hi Andrew,
i originally did so but struct dsa_device_ops is defined inside net/dsa/dsa_priv.h so flow_dissector.c would need a
#include "../dsa/dsa_priv.h"
I was not sure if this is ok or if we would need to move the struct definition to include/net/dsa.h in that case
John
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
quoted
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Would not you usually want to configure RPS at the DSA network device
level where the switch tag has already been popped and you are
processing a regular Ethernet frame at that point?
--
Florian
From: John Crispin <john@phrozen.org> Date: 2017-06-20 17:37:50
On 20/06/17 16:01, Andrew Lunn wrote:
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
quoted
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Andrew
Hi Andrew,
the ethernet driver receives the frame and passes it down the line. Eventually it ends up inside netif_receive_skb_internal() where it gets added to the backlog. At this point get_rps_cpu() is called. Inside get_rps_cpu() the skb_get_hash() is called which utilizes the flow_dissector() ... which is broken for DSA devices. get_rps_cpu() will always return the same hash for all flows and the frame is always added to the backlog on the same core. Once inside the backlog it will traverse through the dsa layer and end up inside the tag driver and be passed to the slave device for further processing and keep its bad flow hash for its whole life cycle.
In theory we could reset the hash inside the tag driver but ideally the whole life cycle of the frame should happen on the same core to avoid possible reordering issues. In addition RPS is broken until the frame reaches the tag driver. In the case of the mediatek mt7623 we only have 1 RX IRQ and in the worst case the RPS of the frame while still inside ethX will happen on the same core as where we handle IRQs. This will increase the IRQ latency and reduce the free cpu time, thus reducing maximum throughput. I did test resetting the hash inside the tag driver. Calculating the correct hash from the start did yield a huge performance difference however, at least on mt7623. We are talking about 30% extra max throughput. This might not be such a big problem if the SoC has a multi queue ethernet core but on mt7623 it does make a huge difference if we can use RPS to delegate all frame processing away from the core handling the IRQs.
John
From: John Crispin <john@phrozen.org> Date: 2017-06-20 17:39:13
On 20/06/17 19:30, Florian Fainelli wrote:
On 06/20/2017 07:01 AM, Andrew Lunn wrote:
quoted
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
quoted
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Would not you usually want to configure RPS at the DSA network device
level where the switch tag has already been popped and you are
processing a regular Ethernet frame at that point?
Hi Florian,
is explained in my mail to Andrew, you really want to be able to setup RPS for all devices in the chain to free up the core handling IRQs
John
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-06-20 21:52:48
On Tue, Jun 20, 2017 at 07:37:35PM +0200, John Crispin wrote:
On 20/06/17 16:01, Andrew Lunn wrote:
quoted
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
quoted
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Andrew
Hi John
I follow your logic of doing the hash early
Is there any value in including the DSA header in the hash? That might
allow frames from different ingress ports to be spread over CPUs?
Andrew
From: John Crispin <john@phrozen.org> Date: 2017-06-21 04:33:32
On 20/06/17 23:52, Andrew Lunn wrote:
quoted
On Tue, Jun 20, 2017 at 07:37:35PM +0200, John Crispin wrote:
On 20/06/17 16:01, Andrew Lunn wrote:
quoted
On Tue, Jun 20, 2017 at 10:06:54AM +0200, John Crispin wrote:
quoted
RPS and probably other kernel features are currently broken on some if not
all DSA devices. The root cause of this that skb_hash will call the
flow_disector.
Hi John
What is the call path when the flow_disector is called? I'm wondering
if we can defer this, and call it later, after the tag code has
removed the header.
Andrew
Hi John
I follow your logic of doing the hash early
Is there any value in including the DSA header in the hash? That might
allow frames from different ingress ports to be spread over CPUs?
Andrew
Hi Andrew,
adding the DSA header wont make any difference and would still require a patch to the flow dissector.
John