Re: [PATCH net-next v2 8/8] netdev: depend on netdev->lock for qstats in ops locked drivers

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 9 Apr 2025 22:23:28 -0700 Jacob Keller wrote:
> > +struct netdev_stat_ops
> > +----------------------
> > +
> > +"qstat" ops are invoked under the instance lock for "ops locked" drivers,
> > +and under rtnl_lock for all other drivers.
> > +
> >  struct net_shaper_ops
> >  ---------------------
> >    
> 
> What determines if a driver is "ops locked"? Is that defined above this
> chunk in the doc? I see its when netdev_need_ops_lock() is set? Ok.

Yup, it was hiding in the previous patch:

   Code comments and docs refer to drivers which have ops called under
   the instance lock as "ops locked".

> Sounds like it would be good to start migrating drivers over to this
> locking paradigm over time.

At least for the drivers which implement queue stats its nice to be able 
to dump stats without taking the global lock. 

> >  	if (ifindex) {
> > -		netdev = __dev_get_by_index(net, ifindex);
> > -		if (netdev && netdev->stat_ops) {
> > +		netdev = netdev_get_by_index_lock_ops_compat(net, ifindex);
> > +		if (!netdev) {
> > +			NL_SET_BAD_ATTR(info->extack,
> > +					info->attrs[NETDEV_A_QSTATS_IFINDEX]);
> > +			return -ENODEV;
> > +		}  
> 
> I guess netdev_get_by_index_lock_ops_compat acquires the lock when it
> returns success?

Yes.

> > +		if (netdev->stat_ops) {
> >  			err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
> >  							    info, ctx);
> >  		} else {
> >  			NL_SET_BAD_ATTR(info->extack,
> >  					info->attrs[NETDEV_A_QSTATS_IFINDEX]);
> > -			err = netdev ? -EOPNOTSUPP : -ENODEV;
> > -		}
> > -	} else {  
> 
> But there's an else branch here so now I'm confused with how this
> locking works.

The diff is really hard to read, sorry, I should have done two patches.
The else branch is _removed_. The code is now:

	if (ifindex) {
		netdev = netdev_get_by_index_lock_ops_compat(net, ifindex);
		...
		netdev_unlock_ops_compat(netdev);  
		return ;
	}

	for_each_lock_scoped() {
	}