Message-ID: <Z2E6XbSFTRC6GfGD@pengutronix.de>
Date: Tue, 17 Dec 2024 09:46:21 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: Vladimir Oltean <olteanv@...il.com>,
	Oleksij Rempel <linux@...pel-privat.de>, netdev@...r.kernel.org,
	andrew@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, nbd@....name,
	sean.wang@...iatek.com, Mark-MC.Lee@...iatek.com,
	lorenzo.bianconi83@...il.com
Subject: Re: [RFC net-next 0/5] Add ETS and TBF Qdisc offload for Airoha
 EN7581 SoC

On Mon, Dec 16, 2024 at 10:44:54PM +0100, Lorenzo Bianconi wrote:
> > On Mon, Dec 16, 2024 at 08:01:33PM +0100, Lorenzo Bianconi wrote:
> > > > On Mon, Dec 16, 2024 at 01:09:01PM +0100, Lorenzo Bianconi wrote:
> > > > > I guess what I did not make clear here is that we are discussing about
> > > > > 'routed' traffic (sorry for that). The traffic is received from the WAN
> > > > > interface and routed to a DSA port (or the other way around).
> > > > > In this scenario the 3-way handshake will be received by the CPU via the
> > > > > WAN port (or the conduit port) while the subsequent packets will be hw
> > > > > forwarded from WAN to LAN (or LAN to WAN). For EN7581 [0], the traffic
> > > > > will be received by the system from GDM2 (WAN) and the PSE/PPE blocks
> > > > > will forward it to the GDM1 port that is connected to the DSA cpu port.
> > > > > 
> > > > > The proposed series is about adding the control path to apply a given Qdisc
> > > > > (ETS or TBF for EN7581) to the traffic that is following the described path
> > > > > without creating it directly on the DSA switch port (for the reasons described
> > > > > before). E.g. the user would want to apply an ETS Qdisc just for traffic
> > > > > egressing via lan0.
> > > > > 
> > > > > This series is not strictly related to the airoha_eth flowtable offload
> > > > > implementation but the latter is required to have a full pictures of the
> > > > > possible use case (this is why I was saying it is better to post it first).
> > > > 
> > > > It's good to know this does not depend on flowtable.
> > > > 
> > > > When you add an offloaded Qdisc to the egress of a net device, you don't
> > > > affect just the traffic L3 routed to that device, but all traffic (also
> > > > includes the packets sent to it using L2 forwarding). As such, I simply
> > > > don't believe that the way in which the UAPI is interpreted here (root
> > > > egress qdisc matches only routed traffic) is proper.
> > > > 
> > > > Ack?
> > > 
> > > Considering patch [0], we are still offloading the Qdisc on the provided
> > > DSA switch port (e.g. LANx) via the port_setup_tc() callback available in
> > > dsa_user_setup_qdisc(), but we are introducing even the ndo_setup_tc_conduit()
> > > callback in order to use the hw Qdisc capabilities available on the mac chip
> > > (e.g. EN7581) for the routed traffic from WAN to LANx. We will still apply
> > > the Qdisc defined on LANx for L2 traffic from LANy to LANx. Agree?
> > > 
> > > > 
> > > > > > I'm trying to look at the big picture and abstract away the flowtable a
> > > > > > bit. I don't think the tc rule should be on the user port. Can the
> > > > > > redirection of packets destined towards a particular switch port be
> > > > > > accomplished with a tc u32 filter on the conduit interface instead?
> > > > > > If the tc primitives for either the filter or the action don't exist,
> > > > > > maybe those could be added instead? Like DSA keys in "flower" which gain
> > > > > > introspection into the encapsulated packet headers?
> > > > > 
> > > > > The issue with the current DSA infrastructure is there is no way to use
> > > > > the conduit port to offload a Qdisc policy to a given lan port since we
> > > > > are missing in the APIs the information about what user port we are
> > > > > interested in (this is why I added the new netdev callback).
> > > > 
> > > > How does the introduction of ndo_setup_tc_conduit() help, since the problem
> > > > is elsewhere? You are not making "tc qdisc add lanN root ets" work correctly.
> > > > It is simply not comparable to the way in which it is offloaded by
> > > > drivers/net/dsa/microchip/ksz_common.c, even though the user space
> > > > syntax is the same. Unless you're suggesting that for ksz it is not
> > > > offloaded correctly?
> > > 
> > > nope, I am not saying the current Qdisc DSA infrastructure is wrong, it just
> > > does not allow to exploit all hw capabilities available on EN7581 when the
> > > traffic is routed from the WAN port to a given DSA switch port. If we do:
> > > 
> > > $tc qdisc add dev lan0 root handle 1: ets strict 8 priomap ...
> > > 
> > > in the current upstream implementation we do:
> > >   dsa_user_setup_tc():
> > >      ...
> > >        -> ds->ops->port_setup_tc(ds, dp->index, type, type_data)
> > >           (it applies the Qdisc on lan0 configuring the hw switch)
> > > 
> > > adding the ndo_setup_tc_conduit() callback we have:
> > > 
> > >   dsa_user_setup_qdisc()
> > >     ...
> > >       -> conduit->netdev_ops->ndo_setup_tc_conduit(conduit, dp->index, type, type_data)
> > >          (it applies the Qdisc on EN7581 mac chip for the routed traffic destinated to lan0)
> > > 
> > >       -> ds->ops->port_setup_tc(ds, dp->index, type, type_data)
> > >          (it applies the Qdisc on lan0 configuring the hw switch)
> > > 
> > > > 
> > > > Oleksij, am I missing something?
> > > > 
> > > > > Please consider here we are discussing about Qdisc policies and not flower
> > > > > rules to mangle the traffic.
> > > > 
> > > > What's a Qdisc policy?
> > > 
> > > I mean a queue scheduler algorithm (e.g. TBF, ETS, HTB, ...)
> > > 
> > > > 
> > > > Also, flower is a classifier, not an action. It doesn't mangle packets
> > > > by the very definition of what a classifier is.
> > > 
> > > yes, but goal of the series is the Queue scheduler offloading, not
> > > classifier/action. Agree?
> > > 
> > > > 
> > > > > The hw needs to be configured in advance to apply the requested policy
> > > > > (e.g TBF for traffic shaping).
> > > > 
> > > > What are you missing exactly to make DSA packets go to a particular
> > > > channel on the conduit?
> > > > 
> > > > For Qdisc offloading you want to configure the NIC in advance, of course.
> > > > 
> > > > Can't you do something like this to guide packets to the correct channels?
> > > > 
> > > > tc qdisc add dev eth0 clsact
> > > > tc qdisc add dev eth0 root handle 1: ets strict 8 priomap ...
> > > > tc filter add dev eth0 egress ${u32 or flower filter to match on DSA tagged packets} \
> > > > 	flowid 1:1
> > > 
> > > If we apply the Qdisc directly on the conduit port (eth0) we can just apply the
> > > queue scheduler on all the traffic egressing via the DSA switch while I would
> > > like to apply it on per DSA port basis (but using the mac chip hw capabilities),
> > > got my point?
> > 
> > Hm, I guess I have similar use case in one of my projects. In my case, the CPU
> > interface is 1Gbit the switch ports are 100Mbit each. It is still
> > possible to keep the CPU interface busy by sending 1Gbit UDP stream, so
> > 900Mbit is dropped by the switch. I would like to have traffic limiter
> > per virtual DSA port on the SoC site to reduce the load on DSA conduit.
> > Currently it was not possible.
> 
> Does the mac chip in your setup support TX shaping (e.g. via HTB or TBF)?
> If so, I guess you could do it via the ndo_setup_tc_conduit() callback.

HW offloading is optional optimization. At first step, it should be able
to work without HW offloading.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists