Re: [PATCHv3 net-next] bonding: add software tx timestamping support

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Jakub Kicinski <kuba@kernel.org> wrote:

>On Mon, 10 Apr 2023 16:23:51 +0800 Hangbin Liu wrote:
>> @@ -5707,10 +5711,38 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
>>  			ret = ops->get_ts_info(real_dev, info);
>>  			goto out;
>>  		}
>> +	} else {
>> +		/* Check if all slaves support software rx/tx timestamping */
>> +		rcu_read_lock();
>> +		bond_for_each_slave_rcu(bond, slave, iter) {
>> +			ret = -1;
>> +			ops = slave->dev->ethtool_ops;
>> +			phydev = slave->dev->phydev;
>> +
>> +			if (phy_has_tsinfo(phydev))
>> +				ret = phy_ts_info(phydev, &ts_info);
>> +			else if (ops->get_ts_info)
>> +				ret = ops->get_ts_info(slave->dev, &ts_info);
>
>Do we _really_ need to hold RCU lock over this?
>Imposing atomic context restrictions on driver callbacks should not be
>taken lightly. I'm 75% sure .ethtool_get_ts_info can only be called
>under rtnl lock off the top of my head, is that not the case?

	Ok, maybe I didn't look at that carefully enough, and now that I
do, it's really complicated.

	Going through it, I think the call path that's relevant is
taprio_change -> taprio_parse_clockid -> ethtool_ops->get_ts_info.
taprio_change is Qdisc_ops.change function, and tc_modify_qdisc should
come in with RTNL held.

	If I'm reading cscope right, the other possible caller of
Qdisc_ops.change is fifo_set_limit, and it looks like that function is
only called by functions that are themselves Qdisc_ops.change functions
(red_change -> __red_change, sfb_change, tbf_change) or Qdisc_ops.init
functions (red_init -> __red_change, sfb_init, tbf_init).

	There's also a qdisc_create_dflt -> Qdisc_ops.init call path,
but I don't know if literally all calls to qdisc_create_dflt hold RTNL.

	There's a lot of them, and I'm not sure how many of those could
ever end up calling into taprio_change (if, say, a taprio qdisc is
attached within another qdisc).

	qdisc_create also calls Qdisc_ops.init, but that one seems to
clearly expect to enter with RTNL.

	Any tc expert able to state for sure whether it's possible to
get into any of the above without RTNL?  I suspect it isn't, but I'm not
100% sure either.


>> +			if (!ret && (ts_info.so_timestamping & SOF_TIMESTAMPING_SOFTRXTX) ==
>> +				    SOF_TIMESTAMPING_SOFTRXTX) {
>
>You could check in this loop if TX is supported...

	I see your point below about not wanting to create
SOFT_TIMESTAMPING_SOFTRXTX, but doesn't the logic need to test all three
of the flags _TX_SOFTWARE, _RX_SOFTWARE, and _SOFTWARE?

	-J

>> +				soft_support = true;
>> +				continue;
>> +			}
>> +
>> +			soft_support = false;
>> +			break;
>> +		}
>> +		rcu_read_unlock();
>>  	}
>>  
>> -	info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>> -				SOF_TIMESTAMPING_SOFTWARE;
>> +	ret = 0;
>> +	if (soft_support) {
>> +		info->so_timestamping = SOF_TIMESTAMPING_SOFTRXTX;
>> +	} else {
>> +		info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>> +					SOF_TIMESTAMPING_SOFTWARE;
>
>...make this unconditional and conditionally add TX...
>
>> +	}
>>  	info->phc_index = -1;
>>  
>>  out:
>> diff --git a/include/uapi/linux/net_tstamp.h b/include/uapi/linux/net_tstamp.h
>> index a2c66b3d7f0f..2adaa0008434 100644
>> --- a/include/uapi/linux/net_tstamp.h
>> +++ b/include/uapi/linux/net_tstamp.h
>> @@ -48,6 +48,9 @@ enum {
>>  					 SOF_TIMESTAMPING_TX_SCHED | \
>>  					 SOF_TIMESTAMPING_TX_ACK)
>>  
>> +#define SOF_TIMESTAMPING_SOFTRXTX (SOF_TIMESTAMPING_TX_SOFTWARE | \
>> +				   SOF_TIMESTAMPING_RX_SOFTWARE | \
>> +				   SOF_TIMESTAMPING_SOFTWARE)
>
>..then you won't need this define in uAPI.

---
	-Jay Vosburgh, jay.vosburgh@canonical.com