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

Hi Andrew,

On Thu, 14 Jan 2021 at 03:03, Andrew Lunn [off-list ref] wrote:

On Thu, Jan 14, 2021 at 02:17:59AM +0200, Vladimir Oltean wrote:

quoted

On Thu, Jan 14, 2021 at 12:41:28AM +0100, Andrew Lunn wrote:

quoted

On Wed, Jan 13, 2021 at 05:41:38PM +0200, Vladimir Oltean wrote:

quoted

+ 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.