Re: [PATCH net-next v2] net: stmmac: improve TSO/GSO queue selection

On Aug 17, Jakub Kicinski wrote:
> On Mon, 17 Aug 2026 04:22:37 +0200 Lorenzo Bianconi wrote:
> > > > thx for the pointers. I reviewed Russell's commits and I guess we have two
> > > > options here:
> > > > 
> > > > - manages all the TSO/GSO checks in ndo_features_check() (stmmac_features_check())
> > > >   and disable TSO/GSO if the selected queue does not support checksum offload
> > > >   or it has TBS enabled. In this case I guess we can drop ndo_select_queue()
> > > >   callback completely (it does not make sense to me to always use queue 0 for
> > > >   TSO/GSO packets, e.g. it does not allow proper mqprio offload).
> > > >   Please note this approach would introduce some performance regressions with
> > > >   respect of the previous implementation.
> > > > - implements TSO/GSO checks in ndo_select_queue() callback
> > > >   (stmmac_select_queue()) in order to keep TSO/GSO enabled if the selected
> > > >   queue supports it and at the same time do not always use queue 0 for TSO/GSO
> > > >   packets (proper qdisc offload). Please note this is patch I am proposing.
> > > > 
> > > > What do you think?  
> > > 
> > > I don't know this driver, or how it's used.
> > > 
> > > From the commit message it sounds like "if the packet wants TSO and 
> > > the queue has a scheduler enabled - take the packet to another queue".
> > > Presumably you know why the TBS is enabled and whether the packet
> > > should or should not be going to that queue in the first place if
> > > you're sending the patch? What is the use case?  
> > 
> > Reviewing the driver codebase, TBS and TSO can't be enabled at the same time
> > for a specific queue (TBS is not enabled via DTS but it is hardcoded in the
> > driver). Moreover, for some devices (e.g. dwmac-intel, dwmac-mediatek, ecc.)
> > TBS is enabled at bootstrap for Q1, Q2 .. Qn, (Q0 is never a TBS queue) while
> > for others (e.g. dwmac-stm32, dwmac-sunxi, ecc.) TBS is not enabled at all.
> > The current codebase forces TSO/GSO packets to Q0 (since the current driver
> > guarantees Q0 is a TSO queue). However this approach does not work with qdisc
> > offload (e.g. mqprio).
> > I think it is clean to just move the TSO logic in ndo_features_check() and drop
> > ndo_select_queue() (previous option 1) but I guess this could introduce a
> > performance regression with respect to the current codebase (we could disable TSO).
> > If we move the TSO logic in ndo_select_queue() (previous option 2), we could avoid
> > forcing the TSO/GSO traffic to Q0 if the selected queue supports it and
> > at the same time avoid any performance regression with respect to the current
> > driver.
> 
> Sounds like we don't have a clear understanding of the Intel / TBS use.
> I'd rather risk introducing a regression for them, and have someone who
> actually knows this system help with the fix than accumulate
> incompatible workarounds.

ack, I am fine with that. I will fix it in v3.

Regards,
Lorenzo

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCaoNfTQAKCRA6cBh0uS2t
rHucAPwNAt0gkFEhUci2GLx+gnpNwFwKJi7Sb5fWvEFWhLzGDgEA16wWdP80EihR
RWpRu6f5NX+XwxLo8nqNpJdEzwzQAQU=
=BORM
-----END PGP SIGNATURE-----