RE: [PATCH v2 net-next 0/7] dpaa2-eth: add support for Rx traffic classes

[Ioana Ciornei <ioana.ciornei@nxp.com>]

> Subject: Re: [PATCH v2 net-next 0/7] dpaa2-eth: add support for Rx traffic
> classes
> 
> On Tue, 19 May 2020 07:38:57 +0000 Ioana Ciornei wrote:
> > > Subject: Re: [PATCH v2 net-next 0/7] dpaa2-eth: add support for Rx
> > > traffic classes
> > >
> > > On Sat, 16 May 2020 08:16:47 +0000 Ioana Ciornei wrote:
> > > > > With the Rx QoS features users won't even be able to tell via
> > > > > standard Linux interfaces what the config was.
> > > >
> > > > Ok, that is true. So how should this information be exported to the user?
> > >
> > > I believe no such interface currently exists.
> >
> > I am having a bit of trouble understanding what should be the route
> > for this feature to get accepted.
> 
> What's the feature you're trying to get accepted? Driver's datapath to behave
> correctly when some proprietary out-of-tree API is used to do the actual
> configuration? Unexciting.
> 

I don't think this comment is very productive. The out-of-tree API has nothing
to do with this. The NIC parameters like number of traffic classes and number of
queues per traffic class are fixed as far as the Linux driver is concerned. 

What this patch set does is make use of the queues corresponding to traffic classes
different than 0, if those exist.

> > Is the problem having the classification to a TC based on the VLAN PCP
> > or is there anything else?
> 
> What you have is basically RX version of mqprio, right? Multiple rings per
> "channel" each gets frames with specific priorities? 

You can think of it like that (maybe, understanding that mqprio cannot be attached
to the ingress qdisc, indeed).
DPAA2 has many peculiarities but I don't think this is one of them. There are some
RX queues, some of which can be given a hardware priority at dequeue time and they
form a traffic class together. The assumption being that you don't want low-priority
traffic to cause congestion for high-priority traffic, to have lower latency for
high-priority traffic, etc.
Some 802.1Q standards like PFC actually depend on having multiple traffic classes too,
based on VLAN PCP.

> This needs to be well
> integrated with the rest of the stack, but I don't think TC qdisc offload is a fit.
> Given we don't have qdiscs on ingress. As I said a new API for this would most
> likely have to be created.

For just assigning a traffic class based on packet headers a tc filter with the
skbedit priority action on ingress is enough (maybe even too much since there are
other drivers that have the same default prioritization based on VLAN PCP).

But you are correct that this would not be enough to cover all possible use cases except
for the most simple ones. There are per-traffic class ingress policers, and those become
tricky to support since there's nothing that denotes the traffic class to match on,
currently. I see 2 possible approaches, each with its own drawbacks:
- Allow clsact to be classful, similar to mqprio, and attach filters to its classes (each
  class would correspond to an ingress traffic class). But this would make the skbedit
  action redundant, since QoS classification with a classful clsact should be done
  completely differently now. Also, the classful clsact would have to deny qdiscs attached
  to it that don't make sense, because all of those were written with egress in mind.
- Try to linearize the ingress filter rules under the classless clsact, both the ones that
  have a skbedit action, and the ones that match on a skb priority in order to perform
  ingress policing. But this would be very brittle because the matching order would depend
  on the order in which the rules were introduced:
  rule 1: flower skb-priority 5 action police rate 34Mbps # note: matching on skb-priority doesn't exist (yet?)
  rule 2: flower vlan_prio 5 action skbedit priority 5
  In this case, traffic with VLAN priority 5 would not get rate-limited to 34Mbps.

So this is one of the reasons why I preferred to defer the hard questions and start with
something simple (which for some reason still gets pushback).

Ioana