[PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

If a metadata dst is present with the type METADATA_HW_PORT_MUX on a dsa cpu
port netdev, assume that it carries the port number and that there is no DSA
tag present in the skb data.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 net/core/flow_dissector.c |  4 +++-
 net/dsa/dsa.c             | 19 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 25cd35f5922e..1f476abc25e1 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -972,11 +972,13 @@ bool __skb_flow_dissect(const struct net *net,
 		if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) &&
 			     proto == htons(ETH_P_XDSA))) {
 			const struct dsa_device_ops *ops;
+			struct metadata_dst *md_dst = skb_metadata_dst(skb);
 			int offset = 0;
 
 			ops = skb->dev->dsa_ptr->tag_ops;
 			/* Only DSA header taggers break flow dissection */
-			if (ops->needed_headroom) {
+			if (ops->needed_headroom &&
+			    (!md_dst || md_dst->type != METADATA_HW_PORT_MUX)) {
 				if (ops->flow_dissect)
 					ops->flow_dissect(skb, &proto, &offset);
 				else
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 64b14f655b23..0b67622cf905 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -11,6 +11,7 @@
 #include <linux/netdevice.h>
 #include <linux/sysfs.h>
 #include <linux/ptp_classify.h>
+#include <net/dst_metadata.h>
 
 #include "dsa_priv.h"
 
@@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
 static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 			  struct packet_type *pt, struct net_device *unused)
 {
+	struct metadata_dst *md_dst = skb_metadata_dst(skb);
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
 	struct sk_buff *nskb = NULL;
 	struct dsa_slave_priv *p;
@@ -229,7 +231,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb)
 		return 0;
 
-	nskb = cpu_dp->rcv(skb, dev);
+	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
+		unsigned int port = md_dst->u.port_info.port_id;
+
+		skb_dst_set(skb, NULL);
+		if (!skb_has_extensions(skb))
+			skb->slow_gro = 0;
+
+		skb->dev = dsa_master_find_slave(dev, 0, port);
+		if (likely(skb->dev)) {
+			dsa_default_offload_fwd_mark(skb);
+			nskb = skb;
+		}
+	} else {
+		nskb = cpu_dp->rcv(skb, dev);
+	}
+
 	if (!nskb) {
 		kfree_skb(skb);
 		return 0;
-- 
2.38.1

[PATCH net-next v3 2/4] net: ethernet: mtk_eth_soc: pass correct VLAN protocol ID to the network stack

[Felix Fietkau]

Use the id from the DMA descriptor instead of hardcoding 802.1q

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 1cf76fd7afbc..891dd6937f89 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1936,7 +1936,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 						htons(RX_DMA_VPID(trxd.rxd4)),
 						RX_DMA_VID(trxd.rxd4));
 			} else if (trxd.rxd2 & RX_DMA_VTAG) {
-				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+				__vlan_hwaccel_put_tag(skb, htons(RX_DMA_VPID(trxd.rxd3)),
 						       RX_DMA_VID(trxd.rxd3));
 			}
 
-- 
2.38.1

[PATCH net-next v3 3/4] net: ethernet: mtk_eth_soc: add support for configuring vlan rx offload

[Felix Fietkau]

Keep the vlan rx offload feature in sync across all netdevs belonging to the
device, since the feature is global and can't be turned off per MAC

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 ++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 891dd6937f89..a118c715b09b 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2724,15 +2724,30 @@ static netdev_features_t mtk_fix_features(struct net_device *dev,
 
 static int mtk_set_features(struct net_device *dev, netdev_features_t features)
 {
-	int err = 0;
+	struct mtk_mac *mac = netdev_priv(dev);
+	struct mtk_eth *eth = mac->hw;
+	netdev_features_t diff = dev->features ^ features;
+	int i;
 
-	if (!((dev->features ^ features) & NETIF_F_LRO))
+	if ((diff & NETIF_F_LRO) && !(features & NETIF_F_LRO))
+		mtk_hwlro_netdev_disable(dev);
+
+	/* Set RX VLAN offloading */
+	if (!(diff & NETIF_F_HW_VLAN_CTAG_RX))
 		return 0;
 
-	if (!(features & NETIF_F_LRO))
-		mtk_hwlro_netdev_disable(dev);
+	mtk_w32(eth, !!(features & NETIF_F_HW_VLAN_CTAG_RX),
+		MTK_CDMP_EG_CTRL);
 
-	return err;
+	/* sync features with other MAC */
+	for (i = 0; i < MTK_MAC_COUNT; i++) {
+		if (!eth->netdev[i] || eth->netdev[i] == dev)
+			continue;
+		eth->netdev[i]->features &= ~NETIF_F_HW_VLAN_CTAG_RX;
+		eth->netdev[i]->features |= features & NETIF_F_HW_VLAN_CTAG_RX;
+	}
+
+	return 0;
 }
 
 /* wait for DMA to finish whatever it is doing before we start using it again */
-- 
2.38.1

[PATCH net-next v3 4/4] net: ethernet: mtk_eth_soc: enable hardware DSA untagging

[Felix Fietkau]

- pass the tag to DSA via metadata dst
- disabled on 7986 for now, since it's not working yet
- disabled if a MAC is enabled that does not use DSA

This improves performance by bypassing the DSA tag driver and avoiding extra
skb data mangling

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 66 ++++++++++++++++++---
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  8 +++
 2 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index a118c715b09b..991ffa0b02f5 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -23,6 +23,7 @@
 #include <linux/jhash.h>
 #include <linux/bitfield.h>
 #include <net/dsa.h>
+#include <net/dst_metadata.h>
 
 #include "mtk_eth_soc.h"
 #include "mtk_wed.h"
@@ -1939,13 +1940,19 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 				__vlan_hwaccel_put_tag(skb, htons(RX_DMA_VPID(trxd.rxd3)),
 						       RX_DMA_VID(trxd.rxd3));
 			}
+		}
+
+		/* When using VLAN untagging in combination with DSA, the
+		 * hardware treats the MTK special tag as a VLAN and untags it.
+		 */
+		if (skb_vlan_tag_present(skb) && netdev_uses_dsa(netdev)) {
+			unsigned int port = ntohs(skb->vlan_proto) & GENMASK(2, 0);
 
-			/* If the device is attached to a dsa switch, the special
-			 * tag inserted in VLAN field by hw switch can * be offloaded
-			 * by RX HW VLAN offload. Clear vlan info.
-			 */
-			if (netdev_uses_dsa(netdev))
-				__vlan_hwaccel_clear_tag(skb);
+			if (port < ARRAY_SIZE(eth->dsa_meta) &&
+			    eth->dsa_meta[port])
+				skb_dst_set_noref(skb, &eth->dsa_meta[port]->dst);
+
+			__vlan_hwaccel_clear_tag(skb);
 		}
 
 		skb_record_rx_queue(skb, 0);
@@ -2990,11 +2997,46 @@ static void mtk_gdm_config(struct mtk_eth *eth, u32 config)
 	mtk_w32(eth, 0, MTK_RST_GL);
 }
 
+
+static bool mtk_uses_dsa(struct net_device *dev)
+{
+#if IS_ENABLED(CONFIG_NET_DSA)
+	return netdev_uses_dsa(dev) &&
+	       dev->dsa_ptr->tag_ops->proto == DSA_TAG_PROTO_MTK;
+#else
+	return false;
+#endif
+}
+
 static int mtk_open(struct net_device *dev)
 {
 	struct mtk_mac *mac = netdev_priv(dev);
 	struct mtk_eth *eth = mac->hw;
-	int err;
+	int i, err;
+
+	if (mtk_uses_dsa(dev)) {
+		for (i = 0; i < ARRAY_SIZE(eth->dsa_meta); i++) {
+			struct metadata_dst *md_dst = eth->dsa_meta[i];
+
+			if (md_dst)
+				continue;
+
+			md_dst = metadata_dst_alloc(0, METADATA_HW_PORT_MUX,
+						    GFP_KERNEL);
+			if (!md_dst)
+				return -ENOMEM;
+
+			md_dst->u.port_info.port_id = i;
+			eth->dsa_meta[i] = md_dst;
+		}
+	} else {
+		/* Hardware special tag parsing needs to be disabled if at least
+		 * one MAC does not use DSA.
+		 */
+		u32 val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
+		val &= ~MTK_CDMP_STAG_EN;
+		mtk_w32(eth, val, MTK_CDMP_IG_CTRL);
+	}
 
 	err = phylink_of_phy_connect(mac->phylink, mac->of_node, 0);
 	if (err) {
@@ -3321,6 +3363,10 @@ static int mtk_hw_init(struct mtk_eth *eth)
 	 */
 	val = mtk_r32(eth, MTK_CDMQ_IG_CTRL);
 	mtk_w32(eth, val | MTK_CDMQ_STAG_EN, MTK_CDMQ_IG_CTRL);
+	if (!MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
+		val = mtk_r32(eth, MTK_CDMP_IG_CTRL);
+		mtk_w32(eth, val | MTK_CDMP_STAG_EN, MTK_CDMP_IG_CTRL);
+	}
 
 	/* Enable RX VLan Offloading */
 	mtk_w32(eth, 1, MTK_CDMP_EG_CTRL);
@@ -3538,6 +3584,12 @@ static int mtk_free_dev(struct mtk_eth *eth)
 		free_netdev(eth->netdev[i]);
 	}
 
+	for (i = 0; i < ARRAY_SIZE(eth->dsa_meta); i++) {
+		if (!eth->dsa_meta[i])
+			break;
+		metadata_dst_free(eth->dsa_meta[i]);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 589f27ddc401..a572416e25de 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -22,6 +22,9 @@
 #include <linux/bpf_trace.h>
 #include "mtk_ppe.h"
 
+#define MTK_MAX_DSA_PORTS	7
+#define MTK_DSA_PORT_MASK	GENMASK(2, 0)
+
 #define MTK_QDMA_PAGE_SIZE	2048
 #define MTK_MAX_RX_LENGTH	1536
 #define MTK_MAX_RX_LENGTH_2K	2048
@@ -91,6 +94,9 @@
 #define MTK_CDMQ_IG_CTRL	0x1400
 #define MTK_CDMQ_STAG_EN	BIT(0)
 
+/* CDMQ Exgress Control Register */
+#define MTK_CDMQ_EG_CTRL	0x1404
+
 /* CDMP Ingress Control Register */
 #define MTK_CDMP_IG_CTRL	0x400
 #define MTK_CDMP_STAG_EN	BIT(0)
@@ -1121,6 +1127,8 @@ struct mtk_eth {
 
 	int				ip_align;
 
+	struct metadata_dst		*dsa_meta[MTK_MAX_DSA_PORTS];
+
 	struct mtk_ppe			*ppe[2];
 	struct rhashtable		flow_table;
 
-- 
2.38.1

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Vladimir Oltean]

On Thu, Nov 10, 2022 at 10:22:08PM +0100, Felix Fietkau wrote:
> If a metadata dst is present with the type METADATA_HW_PORT_MUX on a dsa cpu
> port netdev, assume that it carries the port number and that there is no DSA
> tag present in the skb data.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---

Doesn't look bad to me, but...

>  net/core/flow_dissector.c |  4 +++-
>  net/dsa/dsa.c             | 19 ++++++++++++++++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 25cd35f5922e..1f476abc25e1 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -972,11 +972,13 @@ bool __skb_flow_dissect(const struct net *net,
>  		if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) &&
>  			     proto == htons(ETH_P_XDSA))) {
>  			const struct dsa_device_ops *ops;
> +			struct metadata_dst *md_dst = skb_metadata_dst(skb);
>  			int offset = 0;
>  
>  			ops = skb->dev->dsa_ptr->tag_ops;
>  			/* Only DSA header taggers break flow dissection */
> -			if (ops->needed_headroom) {
> +			if (ops->needed_headroom &&
> +			    (!md_dst || md_dst->type != METADATA_HW_PORT_MUX)) {
>  				if (ops->flow_dissect)
>  					ops->flow_dissect(skb, &proto, &offset);
>  				else
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 64b14f655b23..0b67622cf905 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/sysfs.h>
>  #include <linux/ptp_classify.h>
> +#include <net/dst_metadata.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
>  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  			  struct packet_type *pt, struct net_device *unused)
>  {
> +	struct metadata_dst *md_dst = skb_metadata_dst(skb);
>  	struct dsa_port *cpu_dp = dev->dsa_ptr;
>  	struct sk_buff *nskb = NULL;
>  	struct dsa_slave_priv *p;
> @@ -229,7 +231,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!skb)
>  		return 0;
>  
> -	nskb = cpu_dp->rcv(skb, dev);
> +	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
> +		unsigned int port = md_dst->u.port_info.port_id;
> +
> +		skb_dst_set(skb, NULL);

If you insist on not using the refcounting feature and free your
metadata_dst in the master's remove() function, that's going to
invalidate absolutely any point I'm trying to make. Normally I'd leave
you alone, however I really don't like that this is also forcing DSA to
not use the refcount, and therefore, that it's forcing any other driver
to do the same as mtk_eth_soc. Not sure how that's gonna scale in the
hypothetical future when there will be a DSA master which can offload
RX DSA tags, *and* the switch can change tagging protocols dynamically
(which will force the master to allocate/free its metadata dst's at
runtime too). I guess that will be for me to figure out, which I don't
like.

Jakub, what do you think? Refcounting or no refcounting?

> +		if (!skb_has_extensions(skb))
> +			skb->slow_gro = 0;
> +
> +		skb->dev = dsa_master_find_slave(dev, 0, port);
> +		if (likely(skb->dev)) {
> +			dsa_default_offload_fwd_mark(skb);
> +			nskb = skb;
> +		}
> +	} else {
> +		nskb = cpu_dp->rcv(skb, dev);
> +	}
> +
>  	if (!nskb) {
>  		kfree_skb(skb);
>  		return 0;
> -- 
> 2.38.1
> 

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

On 12.11.22 00:37, Vladimir Oltean wrote:
> On Thu, Nov 10, 2022 at 10:22:08PM +0100, Felix Fietkau wrote:
>> If a metadata dst is present with the type METADATA_HW_PORT_MUX on a dsa cpu
>> port netdev, assume that it carries the port number and that there is no DSA
>> tag present in the skb data.
>> 
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>  				else
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 64b14f655b23..0b67622cf905 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/netdevice.h>
>>  #include <linux/sysfs.h>
>>  #include <linux/ptp_classify.h>
>> +#include <net/dst_metadata.h>
>>  
>>  #include "dsa_priv.h"
>>  
>> @@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
>>  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>>  			  struct packet_type *pt, struct net_device *unused)
>>  {
>> +	struct metadata_dst *md_dst = skb_metadata_dst(skb);
>>  	struct dsa_port *cpu_dp = dev->dsa_ptr;
>>  	struct sk_buff *nskb = NULL;
>>  	struct dsa_slave_priv *p;
>> @@ -229,7 +231,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>>  	if (!skb)
>>  		return 0;
>>  
>> -	nskb = cpu_dp->rcv(skb, dev);
>> +	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
>> +		unsigned int port = md_dst->u.port_info.port_id;
>> +
>> +		skb_dst_set(skb, NULL);
> 
> If you insist on not using the refcounting feature and free your
> metadata_dst in the master's remove() function, that's going to
> invalidate absolutely any point I'm trying to make. Normally I'd leave
> you alone, however I really don't like that this is also forcing DSA to
> not use the refcount, and therefore, that it's forcing any other driver
> to do the same as mtk_eth_soc. Not sure how that's gonna scale in the
> hypothetical future when there will be a DSA master which can offload
> RX DSA tags, *and* the switch can change tagging protocols dynamically
> (which will force the master to allocate/free its metadata dst's at
> runtime too). I guess that will be for me to figure out, which I don't
> like.
> 
> Jakub, what do you think? Refcounting or no refcounting?
If think that refcounting for the metadata dst is useful in this case, I 
can replace the skb_dst_set call with skb_dst_drop() in v4, so it would 
work for both cases.

- Felix

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Jakub Kicinski]

On Sat, 12 Nov 2022 01:37:14 +0200 Vladimir Oltean wrote:
> Jakub, what do you think? Refcounting or no refcounting?

I would not trust my word over Felix's.. but since you asked I'd vote
for refcounted. There's probably a handful of low level redirects
(generic XDP, TC, NFT) that can happen and steal the packet, and
keep it alive for a while. I don't think they will all necessarily
clear the dst.

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

On 12.11.22 05:40, Jakub Kicinski wrote:
> On Sat, 12 Nov 2022 01:37:14 +0200 Vladimir Oltean wrote:
>> Jakub, what do you think? Refcounting or no refcounting?
> 
> I would not trust my word over Felix's.. but since you asked I'd vote
> for refcounted. There's probably a handful of low level redirects
> (generic XDP, TC, NFT) that can happen and steal the packet, and
> keep it alive for a while. I don't think they will all necessarily
> clear the dst.
I don't really see a valid use case in running generic XDP, TC and NFT 
on a DSA master dealing with packets before the tag receive function has 
been run. And after the tag has been processed, the metadata DST is 
cleared from the skb.
How about this: I send a v4 which uses skb_dst_drop instead of 
skb_dst_set, so that other drivers can use refcounting if it makes sense 
for them. For mtk_eth_soc, I prefer to leave out refcounting for 
performance reasons.
Is that acceptable to you guys?

- Felix

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Vladimir Oltean]

On Sat, Nov 12, 2022 at 12:13:15PM +0100, Felix Fietkau wrote:
> On 12.11.22 05:40, Jakub Kicinski wrote:
> I don't really see a valid use case in running generic XDP, TC and NFT on a
> DSA master dealing with packets before the tag receive function has been
> run. And after the tag has been processed, the metadata DST is cleared from
> the skb.

Oh, there are potentially many use cases, the problem is that maybe
there aren't as many actual implementations as ideas? At least XDP is,
I think, expected to be able to deal with DSA tags if run on a DSA
master (not sure how that applies when RX DSA tag is offloaded, but
whatever). Marek Behun had a prototype with Marvell tags, not sure how
far that went in the end:
https://www.mail-archive.com/netdev@vger.kernel.org/msg381018.html
In general, forwarding a packet to another switch port belonging to the
same master via XDP_TX should be relatively efficient.

> How about this: I send a v4 which uses skb_dst_drop instead of skb_dst_set,
> so that other drivers can use refcounting if it makes sense for them. For
> mtk_eth_soc, I prefer to leave out refcounting for performance reasons.
> Is that acceptable to you guys?

I don't think you can mix refcounting at consumer side with no-refcounting
at producer side, no?

I suppose that we could leave refcounting out for now, and bug you if
someone comes with a real need later and complains. Right now it's a bit
hard for me to imagine all the possibilities. How does that sound?

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

On 14.11.22 12:55, Vladimir Oltean wrote:
> On Sat, Nov 12, 2022 at 12:13:15PM +0100, Felix Fietkau wrote:
>> On 12.11.22 05:40, Jakub Kicinski wrote:
>> I don't really see a valid use case in running generic XDP, TC and NFT on a
>> DSA master dealing with packets before the tag receive function has been
>> run. And after the tag has been processed, the metadata DST is cleared from
>> the skb.
> 
> Oh, there are potentially many use cases, the problem is that maybe
> there aren't as many actual implementations as ideas? At least XDP is,
> I think, expected to be able to deal with DSA tags if run on a DSA
> master (not sure how that applies when RX DSA tag is offloaded, but
> whatever). Marek Behun had a prototype with Marvell tags, not sure how
> far that went in the end:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg381018.html
> In general, forwarding a packet to another switch port belonging to the
> same master via XDP_TX should be relatively efficient.
In that case it likely makes sense to disable DSA tag offloading 
whenever driver XDP is being used.
Generic XDP probably doesn't matter much. Last time I tried to use it 
and ran into performance issues, I was told that it's only usable for 
testing anyway and there was no interest in fixing the cases that I ran 
into.

>> How about this: I send a v4 which uses skb_dst_drop instead of skb_dst_set,
>> so that other drivers can use refcounting if it makes sense for them. For
>> mtk_eth_soc, I prefer to leave out refcounting for performance reasons.
>> Is that acceptable to you guys?
> 
> I don't think you can mix refcounting at consumer side with no-refcounting
> at producer side, no?
skb_dst_drop checks if refcounting was used for the skb dst pointer.

> I suppose that we could leave refcounting out for now, and bug you if
> someone comes with a real need later and complains. Right now it's a bit
> hard for me to imagine all the possibilities. How does that sound?
Sounds good. I think I'll send v4 anyway to deal with the XDP case and 
to switch to skb_dst_drop.

- Felix

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Vladimir Oltean]

On Thu, Nov 10, 2022 at 10:22:08PM +0100, Felix Fietkau wrote:
> If a metadata dst is present with the type METADATA_HW_PORT_MUX on a dsa cpu
> port netdev, assume that it carries the port number and that there is no DSA
> tag present in the skb data.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> ---
>  net/core/flow_dissector.c |  4 +++-
>  net/dsa/dsa.c             | 19 ++++++++++++++++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 25cd35f5922e..1f476abc25e1 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -972,11 +972,13 @@ bool __skb_flow_dissect(const struct net *net,
>  		if (unlikely(skb->dev && netdev_uses_dsa(skb->dev) &&
>  			     proto == htons(ETH_P_XDSA))) {
>  			const struct dsa_device_ops *ops;
> +			struct metadata_dst *md_dst = skb_metadata_dst(skb);

Since you're resending, could you please preserve reverse Christmas tree
variable ordering here?

>  			int offset = 0;
>  
>  			ops = skb->dev->dsa_ptr->tag_ops;
>  			/* Only DSA header taggers break flow dissection */
> -			if (ops->needed_headroom) {
> +			if (ops->needed_headroom &&
> +			    (!md_dst || md_dst->type != METADATA_HW_PORT_MUX)) {
>  				if (ops->flow_dissect)
>  					ops->flow_dissect(skb, &proto, &offset);
>  				else
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 64b14f655b23..0b67622cf905 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -11,6 +11,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/sysfs.h>
>  #include <linux/ptp_classify.h>
> +#include <net/dst_metadata.h>
>  
>  #include "dsa_priv.h"
>  
> @@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
>  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  			  struct packet_type *pt, struct net_device *unused)
>  {
> +	struct metadata_dst *md_dst = skb_metadata_dst(skb);
>  	struct dsa_port *cpu_dp = dev->dsa_ptr;
>  	struct sk_buff *nskb = NULL;
>  	struct dsa_slave_priv *p;
> @@ -229,7 +231,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  	if (!skb)
>  		return 0;
>  
> -	nskb = cpu_dp->rcv(skb, dev);
> +	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
> +		unsigned int port = md_dst->u.port_info.port_id;
> +
> +		skb_dst_set(skb, NULL);
> +		if (!skb_has_extensions(skb))
> +			skb->slow_gro = 0;
> +
> +		skb->dev = dsa_master_find_slave(dev, 0, port);
> +		if (likely(skb->dev)) {
> +			dsa_default_offload_fwd_mark(skb);
> +			nskb = skb;
> +		}
> +	} else {
> +		nskb = cpu_dp->rcv(skb, dev);
> +	}
> +
>  	if (!nskb) {
>  		kfree_skb(skb);
>  		return 0;
> -- 
> 2.38.1
> 

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Vladimir Oltean]

On Mon, Nov 14, 2022 at 01:06:42PM +0100, Felix Fietkau wrote:
> In that case it likely makes sense to disable DSA tag offloading whenever
> driver XDP is being used.
> Generic XDP probably doesn't matter much. Last time I tried to use it and
> ran into performance issues, I was told that it's only usable for testing
> anyway and there was no interest in fixing the cases that I ran into.

Don't know about generic XDP performance, sorry. But I think it's
reasonable that XDP programs written for a DSA switch should also work
if the DSA master has no driver support for XDP. At least until it gains
driver level support.

> > > How about this: I send a v4 which uses skb_dst_drop instead of skb_dst_set,
> > > so that other drivers can use refcounting if it makes sense for them. For
> > > mtk_eth_soc, I prefer to leave out refcounting for performance reasons.
> > > Is that acceptable to you guys?
> > 
> > I don't think you can mix refcounting at consumer side with no-refcounting
> > at producer side, no?
> skb_dst_drop checks if refcounting was used for the skb dst pointer.
> 
> > I suppose that we could leave refcounting out for now, and bug you if
> > someone comes with a real need later and complains. Right now it's a bit
> > hard for me to imagine all the possibilities. How does that sound?
> Sounds good. I think I'll send v4 anyway to deal with the XDP case and to
> switch to skb_dst_drop.

Sounds good.

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Dave Taht]

On Mon, Nov 14, 2022 at 4:21 AM Felix Fietkau <nbd@nbd.name> wrote:
>
> On 14.11.22 12:55, Vladimir Oltean wrote:
> > On Sat, Nov 12, 2022 at 12:13:15PM +0100, Felix Fietkau wrote:
> >> On 12.11.22 05:40, Jakub Kicinski wrote:
> >> I don't really see a valid use case in running generic XDP, TC and NFT on a
> >> DSA master dealing with packets before the tag receive function has been
> >> run. And after the tag has been processed, the metadata DST is cleared from
> >> the skb.
> >
> > Oh, there are potentially many use cases, the problem is that maybe
> > there aren't as many actual implementations as ideas? At least XDP is,
> > I think, expected to be able to deal with DSA tags if run on a DSA
> > master (not sure how that applies when RX DSA tag is offloaded, but
> > whatever). Marek Behun had a prototype with Marvell tags, not sure how
> > far that went in the end:
> > https://www.mail-archive.com/netdev@vger.kernel.org/msg381018.html
> > In general, forwarding a packet to another switch port belonging to the
> > same master via XDP_TX should be relatively efficient.
> In that case it likely makes sense to disable DSA tag offloading
> whenever driver XDP is being used.
> Generic XDP probably doesn't matter much. Last time I tried to use it
> and ran into performance issues, I was told that it's only usable for
> testing anyway and there was no interest in fixing the cases that I ran
> into.

XDP continues to evolve rapidly, as do its use cases. ( ex:
ttps://github.com/thebracket/cpumap-pping#readme )

What cases did you run into?

specifically planning to be hacking on the mvpp2 switch driver soon.

>
> >> How about this: I send a v4 which uses skb_dst_drop instead of skb_dst_set,
> >> so that other drivers can use refcounting if it makes sense for them. For
> >> mtk_eth_soc, I prefer to leave out refcounting for performance reasons.
> >> Is that acceptable to you guys?
> >
> > I don't think you can mix refcounting at consumer side with no-refcounting
> > at producer side, no?
> skb_dst_drop checks if refcounting was used for the skb dst pointer.
>
> > I suppose that we could leave refcounting out for now, and bug you if
> > someone comes with a real need later and complains. Right now it's a bit
> > hard for me to imagine all the possibilities. How does that sound?
> Sounds good. I think I'll send v4 anyway to deal with the XDP case and
> to switch to skb_dst_drop.
>
> - Felix



-- 
This song goes out to all the folk that thought Stadia would work:
https://www.linkedin.com/posts/dtaht_the-mushroom-song-activity-6981366665607352320-FXtz
Dave Täht CEO, TekLibre, LLC

Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

On 14.11.22 14:18, Dave Taht wrote:
> On Mon, Nov 14, 2022 at 4:21 AM Felix Fietkau <nbd@nbd.name> wrote:
>>
>> On 14.11.22 12:55, Vladimir Oltean wrote:
>> > On Sat, Nov 12, 2022 at 12:13:15PM +0100, Felix Fietkau wrote:
>> >> On 12.11.22 05:40, Jakub Kicinski wrote:
>> >> I don't really see a valid use case in running generic XDP, TC and NFT on a
>> >> DSA master dealing with packets before the tag receive function has been
>> >> run. And after the tag has been processed, the metadata DST is cleared from
>> >> the skb.
>> >
>> > Oh, there are potentially many use cases, the problem is that maybe
>> > there aren't as many actual implementations as ideas? At least XDP is,
>> > I think, expected to be able to deal with DSA tags if run on a DSA
>> > master (not sure how that applies when RX DSA tag is offloaded, but
>> > whatever). Marek Behun had a prototype with Marvell tags, not sure how
>> > far that went in the end:
>> > https://www.mail-archive.com/netdev@vger.kernel.org/msg381018.html
>> > In general, forwarding a packet to another switch port belonging to the
>> > same master via XDP_TX should be relatively efficient.
>> In that case it likely makes sense to disable DSA tag offloading
>> whenever driver XDP is being used.
>> Generic XDP probably doesn't matter much. Last time I tried to use it
>> and ran into performance issues, I was told that it's only usable for
>> testing anyway and there was no interest in fixing the cases that I ran
>> into.
> 
> XDP continues to evolve rapidly, as do its use cases. ( ex:
> ttps://github.com/thebracket/cpumap-pping#readme )
> 
> What cases did you run into?
My issue was the fact that XDP in general (including generic XDP) 
assumes that packets have 256 bytes of headroom, which is usually only 
true for drivers that already have proper driver or hardware XDP support.
That makes generic XDP pretty much useless for normal use, since on XDP 
capable drivers you should use driver/hardware XDP anyway, and on 
everything else you get the significant performance penalty of an extra 
pskb_expand_head call.
I ended up abandoning XDP for my stuff and used tc instead.

- Felix