Re: [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc

[Stanislav Fomichev <stfomichev@gmail.com>]

On 02/11, Jakub Kicinski wrote:
> On Mon, 10 Feb 2025 11:20:34 -0800 Stanislav Fomichev wrote:
> > Introduce new dev_setup_tc that handles the details and call it from
> > all qdiscs/classifiers. The instance lock is still applied only to
> > the drivers that implement shaper API so only iavf is affected.
> 
> > +int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
> > +		 void *type_data)
> > +{
> > +	const struct net_device_ops *ops = dev->netdev_ops;
> > +
> > +	ASSERT_RTNL();
> > +
> > +	if (tc_can_offload(dev) && ops->ndo_setup_tc) {
> > +		int ret = -ENODEV;
> > +
> > +		if (netif_device_present(dev)) {
> > +			netdev_lock_ops(dev);
> > +			ret = ops->ndo_setup_tc(dev, type, type_data);
> > +			netdev_unlock_ops(dev);
> > +		}
> > +
> > +		return ret;
> > +	}
> > +
> > +	return -EOPNOTSUPP;
> 
> Why the indent? IMHO this would be cleaner:
> 
> 	if (!tc_can_offload || !ops...
> 		return -ENOPNOTSUPP;
> 	if (!netif_device_present(dev))
> 		return -ENODEV:
> 
> 	netdev_lock_ops(dev);
> 	...

I was trying to keep consistent style with existing
dev_siocbond/dev_siocdevprivate/dev_siocwandev. Agreed that non-indented
flow looks much nicer, will use it for dev_setup_tc.

> > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > index 291ab1b4acc4..f2ac7662e4cc 100644
> > --- a/net/dsa/user.c
> > +++ b/net/dsa/user.c
> > @@ -1729,10 +1729,7 @@ static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
> >  {
> >  	struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
> >  
> > -	if (!conduit->netdev_ops->ndo_setup_tc)
> > -		return -EOPNOTSUPP;
> > -
> > -	return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
> > +	return dev_setup_tc(conduit, TC_SETUP_FT, type_data);
> 
> The netfilter / flow table offloads don't seem to test tc_can_offload(),
> should we make that part of the check optional in dev_setup_tc() ?
> Add a bool argument to ignore  tc_can_offload() ?

Let me dig into it... I was assuming that tc_can_offload() is basically
a runtime way to signal to the core that even though the device has
ndo_setup_tc defined, the feature can't be used.

I don't understand why some places care only about ndo_setup_tc
while other test for both ndo_setup_tc/tc_can_offload. Do you by
chance have any context on this? Does tc_can_offload cover only
a subset of (TC_SETUP_BLOCK) the offload types?

The easiest way is probably just to keep calls to tc_can_offload outside,
as is, but I was thinking that doing both ndo_setup_tc and tc_can_offload
is a bit more safe.

> > @@ -855,10 +853,7 @@ void qdisc_offload_graft_helper(struct net_device *dev, struct Qdisc *sch,
> >  	bool any_qdisc_is_offloaded;
> >  	int err;
> >  
> > -	if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> > -		return;
> > -
> > -	err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
> > +	err = dev_setup_tc(dev, type, type_data);
> 
> Probably need to handle -EOPNOTSUPP here now?

Ah, yes, thanks!

> >  	/* Don't report error if the graft is part of destroy operation. */
> >  	if (!err || !new || new == &noop_qdisc)
> 
> > -	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> > +	err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> >  	if (err < 0)
> >  		pr_warn("Couldn't disable CBS offload for queue %d\n",
> >  			cbs.queue);
> > @@ -294,7 +289,7 @@ static int cbs_enable_offload(struct net_device *dev, struct cbs_sched_data *q,
> >  	cbs.idleslope = opt->idleslope;
> >  	cbs.sendslope = opt->sendslope;
> >  
> > -	err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> > +	err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> 
> $ for f in $(git grep --files-with-matches TC_SETUP_QDISC_CBS -- drivers/ ); do \
> 	d=$(dirname $f); \
> 	git grep HW_TC -- $d || echo No match in $d; \
> done
> 
> No match in drivers/net/dsa/microchip
> No match in drivers/net/dsa/ocelot
> No match in drivers/net/dsa/sja1105
> drivers/net/ethernet/freescale/enetc/enetc_pf.c:        if (changed & NETIF_F_HW_TC) {
> drivers/net/ethernet/freescale/enetc/enetc_pf.c:                err = enetc_set_psfp(ndev, !!(features & NETIF_F_HW_TC));
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:         ndev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c:         ndev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/intel/igb/igb_main.c:              netdev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/intel/igc/igc_main.c:      netdev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c:                   NETIF_F_HW_TC;
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c:  dev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:              ndev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/ti/am65-cpsw-nuss.c:                                 NETIF_F_HW_TC;
> drivers/net/ethernet/ti/cpsw_new.c:                               NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_TC;
> 
> 
> Looks like some the these qdiscs will need to ignore the features too :(

Hmm, so TC_SETUP_QDISC_CBS is not influenced by NETIF_F_HW_TC? I guess
it's the same discussion as above... LMK if you have more details.

Thank you for the review, will incorporate the rest of the feedback
from the other replies as well!