[RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall

[RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This is a proposal for configuring the port-based default priority on
switch ports using tc-matchall and skbedit priority. Comments welcome.

Vladimir Oltean (2):
  net: dsa: allow setting port-based QoS priority using tc matchall
    skbedit
  net: dsa: felix: offload port priority

 drivers/net/dsa/ocelot/felix.c | 15 +++++++
 include/net/dsa.h              |  8 ++++
 net/dsa/slave.c                | 72 ++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

-- 
2.25.1

[RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In Time Sensitive Networking it is a common and simple use case to
configure switches to give all traffic from an attached station the same
priority, without requiring those stations to use VLAN PCP or IP DSCP to
signal the priority that they want. Many pieces of hardware support this
feature via a port-based default priority. We can model this in Linux
through a matchall action on the ingress qdisc of the port, plus a
skbedit priority action with the desired priority.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h |  8 ++++++
 net/dsa/slave.c   | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index c9a3dd7588df..4b774287d255 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -155,6 +155,7 @@ struct dsa_switch_tree {
 enum dsa_port_mall_action_type {
 	DSA_PORT_MALL_MIRROR,
 	DSA_PORT_MALL_POLICER,
+	DSA_PORT_MALL_SKBEDIT,
 };
 
 /* TC mirroring entry */
@@ -169,6 +170,10 @@ struct dsa_mall_policer_tc_entry {
 	u64 rate_bytes_per_sec;
 };
 
+struct dsa_mall_skbedit_tc_entry {
+	int priority;
+};
+
 /* TC matchall entry */
 struct dsa_mall_tc_entry {
 	struct list_head list;
@@ -177,6 +182,7 @@ struct dsa_mall_tc_entry {
 	union {
 		struct dsa_mall_mirror_tc_entry mirror;
 		struct dsa_mall_policer_tc_entry policer;
+		struct dsa_mall_skbedit_tc_entry skbedit;
 	};
 };
 
@@ -612,6 +618,8 @@ struct dsa_switch_ops {
 	int	(*port_policer_add)(struct dsa_switch *ds, int port,
 				    struct dsa_mall_policer_tc_entry *policer);
 	void	(*port_policer_del)(struct dsa_switch *ds, int port);
+	int	(*port_priority_set)(struct dsa_switch *ds, int port,
+				     struct dsa_mall_skbedit_tc_entry *skbedit);
 	int	(*port_setup_tc)(struct dsa_switch *ds, int port,
 				 enum tc_setup_type type, void *type_data);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5d7f6cada6a8..82cba26e2a8f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1018,6 +1018,66 @@ dsa_slave_add_cls_matchall_police(struct net_device *dev,
 	return err;
 }
 
+static int
+dsa_slave_add_cls_matchall_skbedit(struct net_device *dev,
+				   struct tc_cls_matchall_offload *cls,
+				   bool ingress)
+{
+	struct netlink_ext_ack *extack = cls->common.extack;
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_mall_skbedit_tc_entry *skbedit;
+	struct dsa_mall_tc_entry *mall_tc_entry;
+	struct dsa_switch *ds = dp->ds;
+	struct flow_action_entry *act;
+	int err;
+
+	if (!ds->ops->port_priority_set) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Port priority not implemented");
+		return -EOPNOTSUPP;
+	}
+
+	if (!ingress) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Only supported on ingress qdisc");
+		return -EOPNOTSUPP;
+	}
+
+	if (!flow_action_basic_hw_stats_check(&cls->rule->action,
+					      cls->common.extack))
+		return -EOPNOTSUPP;
+
+	list_for_each_entry(mall_tc_entry, &p->mall_tc_list, list) {
+		if (mall_tc_entry->type == DSA_PORT_MALL_SKBEDIT) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Only one port priority allowed");
+			return -EEXIST;
+		}
+	}
+
+	act = &cls->rule->action.entries[0];
+
+	mall_tc_entry = kzalloc(sizeof(*mall_tc_entry), GFP_KERNEL);
+	if (!mall_tc_entry)
+		return -ENOMEM;
+
+	mall_tc_entry->cookie = cls->cookie;
+	mall_tc_entry->type = DSA_PORT_MALL_SKBEDIT;
+	skbedit = &mall_tc_entry->skbedit;
+	skbedit->priority = act->priority;
+
+	err = ds->ops->port_priority_set(ds, dp->index, skbedit);
+	if (err) {
+		kfree(mall_tc_entry);
+		return err;
+	}
+
+	list_add_tail(&mall_tc_entry->list, &p->mall_tc_list);
+
+	return err;
+}
+
 static int dsa_slave_add_cls_matchall(struct net_device *dev,
 				      struct tc_cls_matchall_offload *cls,
 				      bool ingress)
@@ -1031,6 +1091,9 @@ static int dsa_slave_add_cls_matchall(struct net_device *dev,
 	else if (flow_offload_has_one_action(&cls->rule->action) &&
 		 cls->rule->action.entries[0].id == FLOW_ACTION_POLICE)
 		err = dsa_slave_add_cls_matchall_police(dev, cls, ingress);
+	else if (flow_offload_has_one_action(&cls->rule->action) &&
+		 cls->rule->action.entries[0].id == FLOW_ACTION_PRIORITY)
+		err = dsa_slave_add_cls_matchall_skbedit(dev, cls, ingress);
 
 	return err;
 }
@@ -1058,6 +1121,15 @@ static void dsa_slave_del_cls_matchall(struct net_device *dev,
 		if (ds->ops->port_policer_del)
 			ds->ops->port_policer_del(ds, dp->index);
 		break;
+	case DSA_PORT_MALL_SKBEDIT:
+		if (ds->ops->port_priority_set) {
+			struct dsa_mall_skbedit_tc_entry *skbedit;
+
+			skbedit = &mall_tc_entry->skbedit;
+			skbedit->priority = 0;
+			ds->ops->port_priority_set(ds, dp->index, skbedit);
+		}
+		break;
 	default:
 		WARN_ON(1);
 	}
-- 
2.25.1

Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit

[Andrew Lunn]

On Wed, Jan 13, 2021 at 05:41:38PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> In Time Sensitive Networking it is a common and simple use case to
> configure switches to give all traffic from an attached station the same
> priority, without requiring those stations to use VLAN PCP or IP DSCP to
> signal the priority that they want. Many pieces of hardware support this
> feature via a port-based default priority. We can model this in Linux
> through a matchall action on the ingress qdisc of the port, plus a
> skbedit priority action with the desired priority.

The mv88e6xxx has something similar. There is a bit to enable this
feature, as well as the priority the feature should have. I think that
then takes a value in the range of 0 to 4, but i could be remembering
it wrongly.

> +	int	(*port_priority_set)(struct dsa_switch *ds, int port,
> +				     struct dsa_mall_skbedit_tc_entry *skbedit);

The fact we can turn this on/off suggests there should be a way to
disable this in the hardware, when the matchall is removed. I don't
see any such remove support in this patch.

    Andrew

Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit

[Vladimir Oltean]

On Thu, Jan 14, 2021 at 12:41:28AM +0100, Andrew Lunn wrote:
> On Wed, Jan 13, 2021 at 05:41:38PM +0200, Vladimir Oltean wrote:
> > +	int	(*port_priority_set)(struct dsa_switch *ds, int port,
> > +				     struct dsa_mall_skbedit_tc_entry *skbedit);
> 
> The fact we can turn this on/off suggests there should be a way to
> disable this in the hardware, when the matchall is removed. I don't
> see any such remove support in this patch.

I don't understand this comment, sorry. When the matchall filter
containing the skbedit action gets removed, DSA calls the driver's
.port_priority_set callback again, this time with a priority of 0.
There's nothing to "remove" about a port priority. I made an assumption
(which I still consider perfectly reasonable) that no port-based
prioritization means that all traffic gets classified to traffic class 0.

Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit

[Andrew Lunn]

On Thu, Jan 14, 2021 at 02:17:59AM +0200, Vladimir Oltean wrote:
> On Thu, Jan 14, 2021 at 12:41:28AM +0100, Andrew Lunn wrote:
> > On Wed, Jan 13, 2021 at 05:41:38PM +0200, Vladimir Oltean wrote:
> > > +	int	(*port_priority_set)(struct dsa_switch *ds, int port,
> > > +				     struct dsa_mall_skbedit_tc_entry *skbedit);
> > 
> > The fact we can turn this on/off suggests there should be a way to
> > disable this in the hardware, when the matchall is removed. I don't
> > see any such remove support in this patch.
> 
> I don't understand this comment, sorry. When the matchall filter
> containing the skbedit action gets removed, DSA calls the driver's
> .port_priority_set callback again, this time with a priority of 0.
> There's nothing to "remove" about a port priority. I made an assumption
> (which I still consider perfectly reasonable) that no port-based
> prioritization means that all traffic gets classified to traffic class 0.

That does not work for mv88e6xxx. Its default setup, if i remember
correctly, is it looks at the TOS bits to determine priority
classes. So in its default state, it is using all the available
traffic classes.  It can also be configured to look at the VLAN
priority, or the TCAM can set the priority class, or there is a per
port default priority, which is what you are describing here. There
are bits to select which of these happen on ingress, on a per port
basis.

So setting the port priority to 0 means setting the priority of
zero. It does not mean go back to the default prioritisation scheme.

I guess any switch which has a range of options for prioritisation
selection will have a similar problem. It defaults to something,
probably something a bit smarter than everything goes to traffic class
0.

      Andrew

[RFC PATCH net-next 2/2] net: dsa: felix: offload port priority

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Even though we should really share the implementation with the ocelot
switchdev driver, that one needs a little bit of rework first, since its
struct ocelot_port_tc only supports one tc matchall action at a time,
which at the moment is used for port policers. Whereas DSA keeps a list
of port-based actions in struct dsa_slave_priv::mall_tc_list, so it is
much more easily extensible. It is too tempting to add the implementation
for the port priority directly in Felix at the moment, which is what we
do.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/ocelot/felix.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 768a74dc462a..5cc42c3aaf0d 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -739,6 +739,20 @@ static void felix_port_policer_del(struct dsa_switch *ds, int port)
 	ocelot_port_policer_del(ocelot, port);
 }
 
+static int felix_port_priority_set(struct dsa_switch *ds, int port,
+				   struct dsa_mall_skbedit_tc_entry *skbedit)
+{
+	struct ocelot *ocelot = ds->priv;
+
+	ocelot_rmw_gix(ocelot,
+		       ANA_PORT_QOS_CFG_QOS_DEFAULT_VAL(skbedit->priority),
+		       ANA_PORT_QOS_CFG_QOS_DEFAULT_VAL_M,
+		       ANA_PORT_QOS_CFG,
+		       port);
+
+	return 0;
+}
+
 static int felix_port_setup_tc(struct dsa_switch *ds, int port,
 			       enum tc_setup_type type,
 			       void *type_data)
@@ -786,6 +800,7 @@ const struct dsa_switch_ops felix_switch_ops = {
 	.port_max_mtu		= felix_get_max_mtu,
 	.port_policer_add	= felix_port_policer_add,
 	.port_policer_del	= felix_port_policer_del,
+	.port_priority_set	= felix_port_priority_set,
 	.cls_flower_add		= felix_cls_flower_add,
 	.cls_flower_del		= felix_cls_flower_del,
 	.cls_flower_stats	= felix_cls_flower_stats,
-- 
2.25.1

Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority

[Andrew Lunn]

On Wed, Jan 13, 2021 at 05:41:39PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Even though we should really share the implementation with the ocelot
> switchdev driver, that one needs a little bit of rework first, since its
> struct ocelot_port_tc only supports one tc matchall action at a time,
> which at the moment is used for port policers. Whereas DSA keeps a list
> of port-based actions in struct dsa_slave_priv::mall_tc_list, so it is
> much more easily extensible. It is too tempting to add the implementation
> for the port priority directly in Felix at the moment, which is what we
> do.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/dsa/ocelot/felix.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 768a74dc462a..5cc42c3aaf0d 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -739,6 +739,20 @@ static void felix_port_policer_del(struct dsa_switch *ds, int port)
>  	ocelot_port_policer_del(ocelot, port);
>  }
>  
> +static int felix_port_priority_set(struct dsa_switch *ds, int port,
> +				   struct dsa_mall_skbedit_tc_entry *skbedit)
> +{
> +	struct ocelot *ocelot = ds->priv;
> +
> +	ocelot_rmw_gix(ocelot,
> +		       ANA_PORT_QOS_CFG_QOS_DEFAULT_VAL(skbedit->priority),

No range check? Seems like -ERANGE or similar would help avoid
surprises when somebody asks for an unsupported priority and it gets
masked to something much lower.

       Andrew

Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority

[Florian Fainelli]

On 1/13/21 3:36 PM, Andrew Lunn wrote:
> On Wed, Jan 13, 2021 at 05:41:39PM +0200, Vladimir Oltean wrote:
>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>>
>> Even though we should really share the implementation with the ocelot
>> switchdev driver, that one needs a little bit of rework first, since its
>> struct ocelot_port_tc only supports one tc matchall action at a time,
>> which at the moment is used for port policers. Whereas DSA keeps a list
>> of port-based actions in struct dsa_slave_priv::mall_tc_list, so it is
>> much more easily extensible. It is too tempting to add the implementation
>> for the port priority directly in Felix at the moment, which is what we
>> do.
>>
>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>> ---
>>  drivers/net/dsa/ocelot/felix.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
>> index 768a74dc462a..5cc42c3aaf0d 100644
>> --- a/drivers/net/dsa/ocelot/felix.c
>> +++ b/drivers/net/dsa/ocelot/felix.c
>> @@ -739,6 +739,20 @@ static void felix_port_policer_del(struct dsa_switch *ds, int port)
>>  	ocelot_port_policer_del(ocelot, port);
>>  }
>>  
>> +static int felix_port_priority_set(struct dsa_switch *ds, int port,
>> +				   struct dsa_mall_skbedit_tc_entry *skbedit)
>> +{
>> +	struct ocelot *ocelot = ds->priv;
>> +
>> +	ocelot_rmw_gix(ocelot,
>> +		       ANA_PORT_QOS_CFG_QOS_DEFAULT_VAL(skbedit->priority),
> 
> No range check? Seems like -ERANGE or similar would help avoid
> surprises when somebody asks for an unsupported priority and it gets
> masked to something much lower.

You are passing the whole dsa_mall_skbedit_tc_entry  structure here,
only to look up priority, would it make sense for now to pass
skbedit->priority as a parameter which would be matching the function
name and what it is dealing with?
-- 
Florian

Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 03:37:49PM -0800, Florian Fainelli wrote:
> You are passing the whole dsa_mall_skbedit_tc_entry  structure here,
> only to look up priority, would it make sense for now to pass
> skbedit->priority as a parameter which would be matching the function
> name and what it is dealing with?

Actually I am passing a pointer to it, which should be more or less
equal in size to an integer. But I can pass just the priority, sure.