* [RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall @ 2021-01-13 15:41 Vladimir Oltean 2021-01-13 15:41 ` [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit Vladimir Oltean 2021-01-13 15:41 ` [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority Vladimir Oltean 0 siblings, 2 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-01-13 15:41 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, netdev Cc: Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, andrew, f.fainelli, vivien.didelot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2021-01-13 15:41 [RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall Vladimir Oltean @ 2021-01-13 15:41 ` Vladimir Oltean 2021-01-13 23:41 ` Andrew Lunn 2021-01-13 15:41 ` [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority Vladimir Oltean 1 sibling, 1 reply; 16+ messages in thread From: Vladimir Oltean @ 2021-01-13 15:41 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, netdev Cc: Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, andrew, f.fainelli, vivien.didelot 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2021-01-13 15:41 ` [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit Vladimir Oltean @ 2021-01-13 23:41 ` Andrew Lunn 2021-01-14 0:17 ` Vladimir Oltean 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2021-01-13 23:41 UTC (permalink / raw) To: Vladimir Oltean Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2021-01-13 23:41 ` Andrew Lunn @ 2021-01-14 0:17 ` Vladimir Oltean 2021-01-14 1:02 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 0:17 UTC (permalink / raw) To: Andrew Lunn Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2021-01-14 0:17 ` Vladimir Oltean @ 2021-01-14 1:02 ` Andrew Lunn 2022-02-10 18:53 ` Vladimir Oltean 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2021-01-14 1:02 UTC (permalink / raw) To: Vladimir Oltean Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2021-01-14 1:02 ` Andrew Lunn @ 2022-02-10 18:53 ` Vladimir Oltean 2022-02-11 7:52 ` Petr Machata 2022-02-11 15:59 ` Andrew Lunn 0 siblings, 2 replies; 16+ messages in thread From: Vladimir Oltean @ 2022-02-10 18:53 UTC (permalink / raw) To: Andrew Lunn Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot Hi Andrew, On Thu, 14 Jan 2021 at 03:03, Andrew Lunn <andrew@lunn.ch> wrote: > 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 I was going through my old patches, and re-reading this conversation, it appears one of us is misunderstanding something. I looked at some Marvell datasheet and it has a similar QoS classification pipeline to Vitesse switches. There is a port-based default priority which can be overridden by IP DSCP, VLAN PCP, or advanced QoS classification (TCAM). The proposal I had was to configure the default port priority using tc matchall skbedit priority. Advanced QoS classification would then be expressed as tc-flower filters with a higher precedence than the matchall (basically the "catchall"). PCP and DSCP, I don't know if that can be expressed cleanly using tc. I think there's something in the dcb ops, but I haven't studied that too deeply. Anyway, I don't exactly understand your point, that an add/del is in any way better than a "set". Even for Marvell, what I'm proposing here would translate in a "set to 0" on "del" anyway. That's why this patch set is RFC. I don't know if there's a better way to express a port-based default priority than a matchall rule having the lowest precedence. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-10 18:53 ` Vladimir Oltean @ 2022-02-11 7:52 ` Petr Machata 2022-02-11 15:29 ` Vladimir Oltean 2022-02-11 15:59 ` Andrew Lunn 1 sibling, 1 reply; 16+ messages in thread From: Petr Machata @ 2022-02-11 7:52 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot Vladimir Oltean <olteanv@gmail.com> writes: > Hi Andrew, > > On Thu, 14 Jan 2021 at 03:03, Andrew Lunn <andrew@lunn.ch> wrote: >> 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 > > I was going through my old patches, and re-reading this conversation, > it appears one of us is misunderstanding something. > > I looked at some Marvell datasheet and it has a similar QoS > classification pipeline to Vitesse switches. There is a port-based > default priority which can be overridden by IP DSCP, VLAN PCP, or > advanced QoS classification (TCAM). > > The proposal I had was to configure the default port priority using tc > matchall skbedit priority. Advanced QoS classification would then be > expressed as tc-flower filters with a higher precedence than the > matchall (basically the "catchall"). PCP and DSCP, I don't know if > that can be expressed cleanly using tc. I think there's something in > the dcb ops, but I haven't studied that too deeply. In 802.1Q-2014, port-default priority is handled as APP entries matching on EtherType of 0. (See Table D-9.) Those are "default priority. For use when priority is not otherwise specified". So DCB ops just handle these as APP entries. Dunno what DSA does. In mlxsw, we call dcb_ieee_getapp_default_prio_mask() when the DCP set_app hook fires to find the relevant entries and get the priority bitmask. Now I don't understand DSA at all, but given a chip with fancy defaults, for the DCB interface in particular, it would make sense to me to have two ops. As long as there are default-prio entries, a "set default priority" op would get invoked with the highest configured default priority. When the last entry disappears, an "unset" op would be called. Not sure what DSA does with ACLs, but it's not clear to me how TC-based prioritization rules coexist with full blown ACLs. I suppose the prio stuff could live on chain 0 and all actions would be skbedit prio pipe goto chain 1 or something. And goto chain 0 is forbidden, because chain 0 is special. Or maybe the prioritization stuff lives on a root qdisc (but no, we need it for ingress packets...) One way or another it looks hairy to dissect and offload accurately IMHO. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-11 7:52 ` Petr Machata @ 2022-02-11 15:29 ` Vladimir Oltean 2022-02-11 16:08 ` Andrew Lunn 2022-02-11 18:24 ` Petr Machata 0 siblings, 2 replies; 16+ messages in thread From: Vladimir Oltean @ 2022-02-11 15:29 UTC (permalink / raw) To: Petr Machata Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot Hi Petr, On Fri, Feb 11, 2022 at 08:52:20AM +0100, Petr Machata wrote: > > Vladimir Oltean <olteanv@gmail.com> writes: > > > Hi Andrew, > > > > On Thu, 14 Jan 2021 at 03:03, Andrew Lunn <andrew@lunn.ch> wrote: > >> 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 > > > > I was going through my old patches, and re-reading this conversation, > > it appears one of us is misunderstanding something. > > > > I looked at some Marvell datasheet and it has a similar QoS > > classification pipeline to Vitesse switches. There is a port-based > > default priority which can be overridden by IP DSCP, VLAN PCP, or > > advanced QoS classification (TCAM). > > > > The proposal I had was to configure the default port priority using tc > > matchall skbedit priority. Advanced QoS classification would then be > > expressed as tc-flower filters with a higher precedence than the > > matchall (basically the "catchall"). PCP and DSCP, I don't know if > > that can be expressed cleanly using tc. I think there's something in > > the dcb ops, but I haven't studied that too deeply. > > In 802.1Q-2014, port-default priority is handled as APP entries matching > on EtherType of 0. (See Table D-9.) Those are "default priority. For use > when priority is not otherwise specified". > > So DCB ops just handle these as APP entries. Dunno what DSA does. In > mlxsw, we call dcb_ieee_getapp_default_prio_mask() when the DCP set_app > hook fires to find the relevant entries and get the priority bitmask. Thanks, these are great pointers. Last time I looked at DCB ops, the dcb iproute program didn't exist, one had to use some LLDP tool IIRC, and it was a bit cumbersome and I dismissed it without even looking at all the details, I didn't notice that the port-default priority corresponds to a selector of 1 and a protocol of 0. The point is that I'm not bent on using tc-matchall for port-based default priority, it's just that I wasn't aware of a better way. But I'll look into adding support for DCB ops for my DSA driver, sounds like a much, much better fit. > Now I don't understand DSA at all, but given a chip with fancy defaults, > for the DCB interface in particular, it would make sense to me to have > two ops. As long as there are default-prio entries, a "set default > priority" op would get invoked with the highest configured default > priority. When the last entry disappears, an "unset" op would be called. I don't understand this comment, sorry. I don't know what's a "chip with fancy defaults". > Not sure what DSA does with ACLs, but it's not clear to me how TC-based > prioritization rules coexist with full blown ACLs. I suppose the prio > stuff could live on chain 0 and all actions would be skbedit prio pipe > goto chain 1 or something. And goto chain 0 is forbidden, because chain > 0 is special. Or maybe the prioritization stuff lives on a root qdisc > (but no, we need it for ingress packets...) One way or another it looks > hairy to dissect and offload accurately IMHO. There's nothing to understand about the DSA core at all, it has no saying in how prioritization or TC rules are configured, that is left down to the hardware driver. To make sure we use the same terminology, when you say "how TC-based prioritization rules coexist with full blown ACLs", you mean trap/drop/redirect by ACLs, right? So the ocelot driver has a programmable, fixed pipeline of multiple ingress stages (VCAP IS1 for VLAN editing and advanced QoS classification) and egress stages (VCAP ES0 for egress VLAN rewriting). We model the entire TCAM subsystem using one chain per TCAM lookup, and force gotos from the current stage to the next. See tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh for the intended usage model. Now, that's all for advanced QoS classification, not for port-based default, VLAN PCP and IP DSCP. My line of thinking is that we could do the latter via dcb-app, and leave the former where it is (skbedit with tc-flower), and they'd coexist just fine, right? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-11 15:29 ` Vladimir Oltean @ 2022-02-11 16:08 ` Andrew Lunn 2022-02-11 18:24 ` Petr Machata 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2022-02-11 16:08 UTC (permalink / raw) To: Vladimir Oltean Cc: Petr Machata, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot > > Now I don't understand DSA at all, but given a chip with fancy defaults, > > for the DCB interface in particular, it would make sense to me to have > > two ops. As long as there are default-prio entries, a "set default > > priority" op would get invoked with the highest configured default > > priority. When the last entry disappears, an "unset" op would be called. > > I don't understand this comment, sorry. I don't know what's a "chip with > fancy defaults". I think this is the point i just razed in my other reply. The hardware starts with a preconfigured QoS profile. My guess would be, a Top Of Rack defaults to all QoS features turned off, you need to turn them off as appropriate for the use case. Typically, a SOHO switch is not configured, it is just put in the corner and turned on. So having some sensible defaults for a home/office makes sense. When it is used in some other embedded use case, it probably is going to get configured for that use cases. So 'fancy defaults' == preconfigured QoS profile in the hardware. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-11 15:29 ` Vladimir Oltean 2022-02-11 16:08 ` Andrew Lunn @ 2022-02-11 18:24 ` Petr Machata 2022-02-17 15:42 ` Vladimir Oltean 1 sibling, 1 reply; 16+ messages in thread From: Petr Machata @ 2022-02-11 18:24 UTC (permalink / raw) To: Vladimir Oltean Cc: Petr Machata, Andrew Lunn, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot Vladimir Oltean <olteanv@gmail.com> writes: > On Fri, Feb 11, 2022 at 08:52:20AM +0100, Petr Machata wrote: >> >> Vladimir Oltean <olteanv@gmail.com> writes: >> >> > Hi Andrew, >> > >> > On Thu, 14 Jan 2021 at 03:03, Andrew Lunn <andrew@lunn.ch> wrote: >> >> 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 >> > >> > I was going through my old patches, and re-reading this conversation, >> > it appears one of us is misunderstanding something. >> > >> > I looked at some Marvell datasheet and it has a similar QoS >> > classification pipeline to Vitesse switches. There is a port-based >> > default priority which can be overridden by IP DSCP, VLAN PCP, or >> > advanced QoS classification (TCAM). >> > >> > The proposal I had was to configure the default port priority using tc >> > matchall skbedit priority. Advanced QoS classification would then be >> > expressed as tc-flower filters with a higher precedence than the >> > matchall (basically the "catchall"). PCP and DSCP, I don't know if >> > that can be expressed cleanly using tc. I think there's something in >> > the dcb ops, but I haven't studied that too deeply. >> >> In 802.1Q-2014, port-default priority is handled as APP entries matching >> on EtherType of 0. (See Table D-9.) Those are "default priority. For use >> when priority is not otherwise specified". >> >> So DCB ops just handle these as APP entries. Dunno what DSA does. In >> mlxsw, we call dcb_ieee_getapp_default_prio_mask() when the DCP set_app >> hook fires to find the relevant entries and get the priority bitmask. > > Thanks, these are great pointers. Last time I looked at DCB ops, the dcb > iproute program didn't exist, one had to use some LLDP tool IIRC, and it > was a bit cumbersome and I dismissed it without even looking at all the > details, I didn't notice that the port-default priority corresponds to a > selector of 1 and a protocol of 0. > > The point is that I'm not bent on using tc-matchall for port-based > default priority, it's just that I wasn't aware of a better way. > But I'll look into adding support for DCB ops for my DSA driver, sounds > like a much, much better fit. > >> Now I don't understand DSA at all, but given a chip with fancy defaults, >> for the DCB interface in particular, it would make sense to me to have >> two ops. As long as there are default-prio entries, a "set default >> priority" op would get invoked with the highest configured default >> priority. When the last entry disappears, an "unset" op would be called. > > I don't understand this comment, sorry. I don't know what's a "chip with > fancy defaults". I'm referring here to Andrew's "I guess any switch [...] defaults to something [...] a bit smarter than everything goes to traffic class 0". >> Not sure what DSA does with ACLs, but it's not clear to me how TC-based >> prioritization rules coexist with full blown ACLs. I suppose the prio >> stuff could live on chain 0 and all actions would be skbedit prio pipe >> goto chain 1 or something. And goto chain 0 is forbidden, because chain >> 0 is special. Or maybe the prioritization stuff lives on a root qdisc >> (but no, we need it for ingress packets...) One way or another it looks >> hairy to dissect and offload accurately IMHO. > > There's nothing to understand about the DSA core at all, it has no > saying in how prioritization or TC rules are configured, that is left > down to the hardware driver. > > To make sure we use the same terminology, when you say "how TC-based > prioritization rules coexist with full blown ACLs", you mean > trap/drop/redirect by ACLs, right? Yeah. But also simple stuff, like skbedit priority, but with complex matching. Think flower match on a side chain that only gets invoked when another flower match hits. > So the ocelot driver has a programmable, fixed pipeline of multiple > ingress stages (VCAP IS1 for VLAN editing and advanced QoS classification) > and egress stages (VCAP ES0 for egress VLAN rewriting). We model the > entire TCAM subsystem using one chain per TCAM lookup, and force gotos > from the current stage to the next. See > tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh for the > intended usage model. > > Now, that's all for advanced QoS classification, not for port-based > default, VLAN PCP and IP DSCP. My line of thinking is that we could do > the latter via dcb-app, and leave the former where it is (skbedit with > tc-flower), and they'd coexist just fine, right? That's what we do. I don't like it very much, because DCB is this odd HW-centric thing that you can't run on bridged veths. But unfortunately TC filter configuration that describes the dumb stuff and then follows up with more of the complex stuff that needs to happen _as well_, seems like it would be a mess to both dissect in the driver and use on the command line. Maybe we need a multi-stage clsact qdisc, or something like that... ^o^ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-11 18:24 ` Petr Machata @ 2022-02-17 15:42 ` Vladimir Oltean 0 siblings, 0 replies; 16+ messages in thread From: Vladimir Oltean @ 2022-02-17 15:42 UTC (permalink / raw) To: Petr Machata Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot Hi Petr, On Fri, Feb 11, 2022 at 07:24:34PM +0100, Petr Machata wrote: > >> Now I don't understand DSA at all, but given a chip with fancy defaults, > >> for the DCB interface in particular, it would make sense to me to have > >> two ops. As long as there are default-prio entries, a "set default > >> priority" op would get invoked with the highest configured default > >> priority. When the last entry disappears, an "unset" op would be called. > > > > I don't understand this comment, sorry. I don't know what's a "chip with > > fancy defaults". > > I'm referring here to Andrew's "I guess any switch [...] defaults to > something [...] a bit smarter than everything goes to traffic class 0". > > >> Not sure what DSA does with ACLs, but it's not clear to me how TC-based > >> prioritization rules coexist with full blown ACLs. I suppose the prio > >> stuff could live on chain 0 and all actions would be skbedit prio pipe > >> goto chain 1 or something. And goto chain 0 is forbidden, because chain > >> 0 is special. Or maybe the prioritization stuff lives on a root qdisc > >> (but no, we need it for ingress packets...) One way or another it looks > >> hairy to dissect and offload accurately IMHO. > > > > There's nothing to understand about the DSA core at all, it has no > > saying in how prioritization or TC rules are configured, that is left > > down to the hardware driver. > > > > To make sure we use the same terminology, when you say "how TC-based > > prioritization rules coexist with full blown ACLs", you mean > > trap/drop/redirect by ACLs, right? > > Yeah. But also simple stuff, like skbedit priority, but with complex > matching. Think flower match on a side chain that only gets invoked when > another flower match hits. > > > So the ocelot driver has a programmable, fixed pipeline of multiple > > ingress stages (VCAP IS1 for VLAN editing and advanced QoS classification) > > and egress stages (VCAP ES0 for egress VLAN rewriting). We model the > > entire TCAM subsystem using one chain per TCAM lookup, and force gotos > > from the current stage to the next. See > > tools/testing/selftests/drivers/net/ocelot/tc_flower_chains.sh for the > > intended usage model. > > > > Now, that's all for advanced QoS classification, not for port-based > > default, VLAN PCP and IP DSCP. My line of thinking is that we could do > > the latter via dcb-app, and leave the former where it is (skbedit with > > tc-flower), and they'd coexist just fine, right? > > That's what we do. I don't like it very much, because DCB is this odd > HW-centric thing that you can't run on bridged veths. But unfortunately > TC filter configuration that describes the dumb stuff and then follows > up with more of the complex stuff that needs to happen _as well_, seems > like it would be a mess to both dissect in the driver and use on the > command line. > > Maybe we need a multi-stage clsact qdisc, or something like that... ^o^ I see dcb_ieee_setapp() can be used to preload the Application Priority Table with information that reflects the port's configuration. I'm just wondering - do you have any idea why the Application Priority TLV doesn't have a way to describe a mapping between VLAN PCP and priority? What can I use to also describe that? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit 2022-02-10 18:53 ` Vladimir Oltean 2022-02-11 7:52 ` Petr Machata @ 2022-02-11 15:59 ` Andrew Lunn 1 sibling, 0 replies; 16+ messages in thread From: Andrew Lunn @ 2022-02-11 15:59 UTC (permalink / raw) To: Vladimir Oltean Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot On Thu, Feb 10, 2022 at 08:53:21PM +0200, Vladimir Oltean wrote: > Hi Andrew, > > On Thu, 14 Jan 2021 at 03:03, Andrew Lunn <andrew@lunn.ch> wrote: > > 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 > > I was going through my old patches, and re-reading this conversation, > it appears one of us is misunderstanding something. > > I looked at some Marvell datasheet and it has a similar QoS > classification pipeline to Vitesse switches. There is a port-based > default priority which can be overridden by IP DSCP, VLAN PCP, or > advanced QoS classification (TCAM). > > The proposal I had was to configure the default port priority using tc > matchall skbedit priority. Advanced QoS classification would then be > expressed as tc-flower filters with a higher precedence than the > matchall (basically the "catchall"). PCP and DSCP, I don't know if > that can be expressed cleanly using tc. I think there's something in > the dcb ops, but I haven't studied that too deeply. > > Anyway, I don't exactly understand your point, that an add/del is in > any way better than a "set". Even for Marvell, what I'm proposing here > would translate in a "set to 0" on "del" anyway. That's why this patch > set is RFC. I don't know if there's a better way to express a > port-based default priority than a matchall rule having the lowest > precedence. I think we have a generic problem in that the switch does not start up in a state where all QoS features are turned off. But a traditional netdev does have all QoS features off by default. You need to explicitly turn on a QoS feature on a netdev by using tc, or some other configuration mechanism. To make the linux view of QoS features actually match what the hardware is doing, we need to preload tc with a number of rules. Your proposed 'tc matchall skbedit priority' rule might need to be already in tc because the switch might already be doing that by default, etc. I also wonder if we need tc rules you cannot actually remove, because you cannot turn the feature off in hardware? 'tc matchall skbedit priority' is setting the default priority. If you remove the rule, the hardware is still going to apply a default priority, it is just ambiguous from tc what value it is using. It would be better if the rule was present from boot, and all you can do is change the priority, not remove the rule. We need to consider the generic problem that the hardware comes with a preconfigured QoS profile, which we currently don't reflect in Linux. We have deployed devices which rely on that QoS profile. How do we transition to describing that preconfigured QoS profile, and allowing it to be changed? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority 2021-01-13 15:41 [RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall Vladimir Oltean 2021-01-13 15:41 ` [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit Vladimir Oltean @ 2021-01-13 15:41 ` Vladimir Oltean 2021-01-13 23:36 ` Andrew Lunn 1 sibling, 1 reply; 16+ messages in thread From: Vladimir Oltean @ 2021-01-13 15:41 UTC (permalink / raw) To: David S. Miller, Jakub Kicinski, netdev Cc: Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, andrew, f.fainelli, vivien.didelot 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority 2021-01-13 15:41 ` [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority Vladimir Oltean @ 2021-01-13 23:36 ` Andrew Lunn 2021-01-13 23:37 ` Florian Fainelli 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2021-01-13 23:36 UTC (permalink / raw) To: Vladimir Oltean Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, f.fainelli, vivien.didelot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority 2021-01-13 23:36 ` Andrew Lunn @ 2021-01-13 23:37 ` Florian Fainelli 2021-01-14 0:57 ` Vladimir Oltean 0 siblings, 1 reply; 16+ messages in thread From: Florian Fainelli @ 2021-01-13 23:37 UTC (permalink / raw) To: Andrew Lunn, Vladimir Oltean Cc: David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, vivien.didelot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority 2021-01-13 23:37 ` Florian Fainelli @ 2021-01-14 0:57 ` Vladimir Oltean 0 siblings, 0 replies; 16+ messages in thread From: Vladimir Oltean @ 2021-01-14 0:57 UTC (permalink / raw) To: Florian Fainelli Cc: Andrew Lunn, David S. Miller, Jakub Kicinski, netdev, Ido Schimmel, Petr Machata, Alexander Duyck, Jamal Hadi Salim, Cong Wang, Jiri Pirko, vivien.didelot 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-02-17 15:43 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 15:41 [RFC PATCH net-next 0/2] Port-based priority on DSA switches using tc-matchall Vladimir Oltean 2021-01-13 15:41 ` [RFC PATCH net-next 1/2] net: dsa: allow setting port-based QoS priority using tc matchall skbedit Vladimir Oltean 2021-01-13 23:41 ` Andrew Lunn 2021-01-14 0:17 ` Vladimir Oltean 2021-01-14 1:02 ` Andrew Lunn 2022-02-10 18:53 ` Vladimir Oltean 2022-02-11 7:52 ` Petr Machata 2022-02-11 15:29 ` Vladimir Oltean 2022-02-11 16:08 ` Andrew Lunn 2022-02-11 18:24 ` Petr Machata 2022-02-17 15:42 ` Vladimir Oltean 2022-02-11 15:59 ` Andrew Lunn 2021-01-13 15:41 ` [RFC PATCH net-next 2/2] net: dsa: felix: offload port priority Vladimir Oltean 2021-01-13 23:36 ` Andrew Lunn 2021-01-13 23:37 ` Florian Fainelli 2021-01-14 0:57 ` Vladimir Oltean
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).