[PATCH net-next] dsa: Make offloading optional on per port basis

[PATCH net-next] dsa: Make offloading optional on per port basis

[Andy Strohman]

The author has a couple use cases for this:

1) Creating a sniffer, or ethernet tap, by bridging two or more
non-offloaded ports to the bridge, and tcpdump'ing the member
ports. Along the same lines, it would be nice to have the ability
to temporarily disable offloading to sniff all traffic for debugging.

2) Work around bugs in the hardware switch or use features
that are only available in software.

DSA drivers can be modified to remove their port_bridge_join()
dsa_switch_ops member to accomplish this. But, it would be better
to make it this runtime configurable, and configurable on a per port
basis.

The key to signaling that a port is not offloading is by
ensuring dp->bridge == NULL. With this, the VLAN and FDB
operations that affect hardware (ie port_fdb_add, port_vlan_del, etc)
will not run. dsa_user_fdb_event() will bail if !dp->bridge.
dsa_user_port_obj_add() checks dsa_port_offloads_bridge_port(),
and dsa_user_host_vlan_add() checks !dp->bridge.

By being configurable on a per port basis (as opposed to switch-wide),
we can have some subset of a switch's ports offloading and others not.

While this approach is generic, and therefore will be available for all
dsa switches, I have only tested this on a mt7530 switch. It may not be
possible or feasible to disable offloading on other switches.

A flags member was added to the dsa user port netdev private data structure
in order to facilitate adding future dsa specific flags more easily.
IFLA_VLAN_FLAGS was used as an example when implementing the flags member.

Signed-off-by: Andy Strohman <andrew@andrewstrohman.com>
---
 include/uapi/linux/if_link.h       | 10 ++++++++
 net/dsa/netlink.c                  | 38 ++++++++++++++++++++++++++++++
 net/dsa/switch.c                   |  3 ++-
 net/dsa/user.h                     |  2 ++
 tools/include/uapi/linux/if_link.h |  1 +
 5 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index f7f638f25020..c5e89064d48c 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1972,9 +1972,19 @@ enum {
 	IFLA_DSA_CONDUIT,
 	/* Deprecated, use IFLA_DSA_CONDUIT instead */
 	IFLA_DSA_MASTER = IFLA_DSA_CONDUIT,
+	IFLA_DSA_FLAGS,
 	__IFLA_DSA_MAX,
 };
 
 #define IFLA_DSA_MAX	(__IFLA_DSA_MAX - 1)
 
+struct ifla_dsa_flags {
+	__u32   flags;
+	__u32   mask;
+};
+
+enum dsa_flags {
+	DSA_FLAG_OFFLOADING_DISABLED    = 0x1,
+};
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/dsa/netlink.c b/net/dsa/netlink.c
index 1332e56349e5..376ba00957fe 100644
--- a/net/dsa/netlink.c
+++ b/net/dsa/netlink.c
@@ -9,13 +9,35 @@
 
 static const struct nla_policy dsa_policy[IFLA_DSA_MAX + 1] = {
 	[IFLA_DSA_CONDUIT]	= { .type = NLA_U32 },
+	[IFLA_DSA_FLAGS]	= { .len = sizeof(struct ifla_dsa_flags) },
 };
 
+static int dsa_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
+{
+	struct dsa_user_priv *p = netdev_priv(dev);
+	u32 old_flags = p->flags;
+
+	/* For now, we only support make these changes when the port is not a member
+	 * of a bridge (ie in standalone mode). If the user wants to alter these flags
+	 * for ports that are currently members of a bridge need to first remove the
+	 * interface from the bridge. Then they can add interface back
+	 * after making their desired flag changes.
+	 */
+
+	if (netif_is_bridge_port(dev))
+		return -EBUSY;
+
+	p->flags = (old_flags & ~mask) | (flags & mask);
+
+	return 0;
+}
+
 static int dsa_changelink(struct net_device *dev, struct nlattr *tb[],
 			  struct nlattr *data[],
 			  struct netlink_ext_ack *extack)
 {
 	int err;
+	struct ifla_dsa_flags *flags;
 
 	if (!data)
 		return 0;
@@ -32,6 +54,12 @@ static int dsa_changelink(struct net_device *dev, struct nlattr *tb[],
 		if (err)
 			return err;
 	}
+	if (data[IFLA_DSA_FLAGS]) {
+		flags = nla_data(data[IFLA_DSA_FLAGS]);
+		err = dsa_dev_change_flags(dev, flags->flags, flags->mask);
+		if (err)
+			return err;
+	}
 
 	return 0;
 }
@@ -45,10 +73,20 @@ static size_t dsa_get_size(const struct net_device *dev)
 static int dsa_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct net_device *conduit = dsa_user_to_conduit(dev);
+	struct dsa_user_priv *dsa = netdev_priv(dev);
+	struct ifla_dsa_flags f;
+
 
 	if (nla_put_u32(skb, IFLA_DSA_CONDUIT, conduit->ifindex))
 		return -EMSGSIZE;
 
+	if (dsa->flags) {
+		f.flags = dsa->flags;
+		f.mask = ~0;
+		if (nla_put(skb, IFLA_DSA_FLAGS, sizeof(f), &f))
+			return -EMSGSIZE;
+	}
+
 	return 0;
 }
 
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 3d2feeea897b..f0bb354c3961 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -83,9 +83,10 @@ static int dsa_switch_bridge_join(struct dsa_switch *ds,
 				  struct dsa_notifier_bridge_info *info)
 {
 	int err;
+	struct dsa_user_priv *priv = netdev_priv(info->dp->user);
 
 	if (info->dp->ds == ds) {
-		if (!ds->ops->port_bridge_join)
+		if (!ds->ops->port_bridge_join || priv->flags & DSA_FLAG_OFFLOADING_DISABLED)
 			return -EOPNOTSUPP;
 
 		err = ds->ops->port_bridge_join(ds, info->dp->index,
diff --git a/net/dsa/user.h b/net/dsa/user.h
index 016884bead3c..846af7ed819b 100644
--- a/net/dsa/user.h
+++ b/net/dsa/user.h
@@ -33,6 +33,8 @@ struct dsa_user_priv {
 
 	/* TC context */
 	struct list_head	mall_tc_list;
+
+	u32			flags;
 };
 
 void dsa_user_mii_bus_init(struct dsa_switch *ds);
diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index 8516c1ccd57a..0982b309b09c 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -1970,6 +1970,7 @@ enum {
 	IFLA_DSA_CONDUIT,
 	/* Deprecated, use IFLA_DSA_CONDUIT instead */
 	IFLA_DSA_MASTER = IFLA_DSA_CONDUIT,
+	IFLA_DSA_FLAGS,
 	__IFLA_DSA_MAX,
 };
 
-- 
2.34.1

Re: [PATCH net-next] dsa: Make offloading optional on per port basis

[Jiri Pirko]

Sun, Dec 01, 2024 at 08:42:11AM CET, andrew@andrewstrohman.com wrote:
>The author has a couple use cases for this:
>
>1) Creating a sniffer, or ethernet tap, by bridging two or more
>non-offloaded ports to the bridge, and tcpdump'ing the member
>ports. Along the same lines, it would be nice to have the ability
>to temporarily disable offloading to sniff all traffic for debugging.
>
>2) Work around bugs in the hardware switch or use features
>that are only available in software.
>
>DSA drivers can be modified to remove their port_bridge_join()
>dsa_switch_ops member to accomplish this. But, it would be better
>to make it this runtime configurable, and configurable on a per port
>basis.
>
>The key to signaling that a port is not offloading is by
>ensuring dp->bridge == NULL. With this, the VLAN and FDB
>operations that affect hardware (ie port_fdb_add, port_vlan_del, etc)
>will not run. dsa_user_fdb_event() will bail if !dp->bridge.
>dsa_user_port_obj_add() checks dsa_port_offloads_bridge_port(),
>and dsa_user_host_vlan_add() checks !dp->bridge.
>
>By being configurable on a per port basis (as opposed to switch-wide),
>we can have some subset of a switch's ports offloading and others not.
>
>While this approach is generic, and therefore will be available for all
>dsa switches, I have only tested this on a mt7530 switch. It may not be
>possible or feasible to disable offloading on other switches.
>
>A flags member was added to the dsa user port netdev private data structure
>in order to facilitate adding future dsa specific flags more easily.
>IFLA_VLAN_FLAGS was used as an example when implementing the flags member.
>
>Signed-off-by: Andy Strohman <andrew@andrewstrohman.com>

Why is this DSA specific? Plus, you say you want to disable offloading
in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only
when joining bridge. I mean, shouldn't this be rather something exposed
by some common UAPI?

Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for?

Re: [PATCH net-next] dsa: Make offloading optional on per port basis

[Vladimir Oltean]

On Mon, Dec 02, 2024 at 10:27:25AM +0100, Jiri Pirko wrote:
> Why is this DSA specific? Plus, you say you want to disable offloading
> in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only
> when joining bridge. I mean, shouldn't this be rather something exposed
> by some common UAPI?

I agree with this. The proposed functionality isn't DSA specific, and
thus, the UAPI to configure it shouldn't be made so.

> Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for?

Is it? macvlan uses NETIF_F_HW_L2FW_DOFFLOAD to detect presence of
netdev_ops->ndo_dfwd_add_station(). Having to even consider macvlan
offload and its implications just because somebody decided to monopolize
the "l2-fwd-offload" name seems at least bizarre in my opinion.

Re: [PATCH net-next] dsa: Make offloading optional on per port basis

[Andrew Strohman]

Hi Jiri,

 Thanks for the review.

> Why is this DSA specific?

I can make this more general. I'm not aware of potential
users outside of switchdev and dsa. Are you anticipating
additional users outside of switchdev?

I could make this more general, and just implement
for dsa for now. Then later, if someone else wants
this functionality for switchdev, or something else,
they could implement that part.


> Plus, you say you want to disable offloading
> in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only
> when joining bridge.

I think it's only required for joining a bridge because
that's where dp->bridge gets assigned. All the other
offloading related code paths check dp->bridge
either directly or indirectly, to determine if the port
is offloaded or not before continuing. If you see
an offloading related code path that does not
consider dp->bridge before moving forward,
please let me know.


> I mean, shouldn't this be rather something exposed
> by some common UAPI?
>
> Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for?

It sounds like Vladimir doesn't like this suggestion. So,
I considered introducing another netdev feature for this, but
I noticed that we are currently maxed out since
netdev_features_t is u64 and NETDEV_FEATURE_COUNT
is 64.

I considered changing netdev_features_t to a bitmap,
so that we can keep adding additional features, but
there are users doing bitwise operations directly on
instances of netdev_features_t, so that seems difficult
to untangle.

Do you have a suggestion about how to proceed?
How should I signal that offloading should be disabled?

Re: [PATCH net-next] dsa: Make offloading optional on per port basis

[Andrew Strohman]

Hi Vladimir,

Thanks for the review. I've responded back to Jiri,
about making this more generic. I'm happy
to make this more generic, but I'd appreciate
some guidance on the best way forward.

So, if you have a suggestion about how to store
the configuration that signals that the port should
not be offloaded, I'd love to hear it.

As I described in my response to Jiri, it looks like
adding the next netdev feature will be painful.
Do you thinking adding a new netdev feature
is the best way forward here?


Thanks,

Andy





On Mon, Dec 2, 2024 at 1:45 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Mon, Dec 02, 2024 at 10:27:25AM +0100, Jiri Pirko wrote:
> > Why is this DSA specific? Plus, you say you want to disable offloading
> > in general (DSA_FLAG_OFFLOADING_DISABLED), but you check the flag only
> > when joining bridge. I mean, shouldn't this be rather something exposed
> > by some common UAPI?
>
> I agree with this. The proposed functionality isn't DSA specific, and
> thus, the UAPI to configure it shouldn't be made so.
>
> > Btw, isn't NETIF_F_HW_L2FW_DOFFLOAD what you are looking for?
>
> Is it? macvlan uses NETIF_F_HW_L2FW_DOFFLOAD to detect presence of
> netdev_ops->ndo_dfwd_add_station(). Having to even consider macvlan
> offload and its implications just because somebody decided to monopolize
> the "l2-fwd-offload" name seems at least bizarre in my opinion.