Re: [PATCH net-next 06/10] net: sched: conditionally obtain rtnl lock in cls hw offloads API

[Vlad Buslov <vladbu@mellanox.com>]

On Fri 23 Aug 2019 at 02:03, Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> On Thu, 22 Aug 2019 15:43:49 +0300, Vlad Buslov wrote:
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 02a547aa77c0..bda42f1b5514 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -3076,11 +3076,28 @@ __tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>>  int tc_setup_cb_call(struct tcf_block *block, enum tc_setup_type type,
>>  		     void *type_data, bool err_stop, bool rtnl_held)
>>  {
>> +	bool take_rtnl = false;
>
> Should we perhaps speculatively:
>
> 	 bool take_rtnl = READ_ONCE(block->lockeddevcnt);
>
> here? It shouldn't hurt, really, and otherwise every offload that
> requires rtnl will have to re-lock cb_lock, every single time..

Great idea! This looks like a straightforward opportunity for
optimization.

>
>>  	int ok_count;
>>
>> +retry:
>> +	if (take_rtnl)
>> +		rtnl_lock();
>>  	down_read(&block->cb_lock);
>> +	/* Need to obtain rtnl lock if block is bound to devs that require it.
>> +	 * In block bind code cb_lock is obtained while holding rtnl, so we must
>> +	 * obtain the locks in same order here.
>> +	 */
>> +	if (!rtnl_held && !take_rtnl && block->lockeddevcnt) {
>> +		up_read(&block->cb_lock);
>> +		take_rtnl = true;
>> +		goto retry;
>> +	}
>> +
>>  	ok_count = __tc_setup_cb_call(block, type, type_data, err_stop);
>> +
>>  	up_read(&block->cb_lock);
>> +	if (take_rtnl)
>> +		rtnl_unlock();
>>  	return ok_count;
>>  }
>>  EXPORT_SYMBOL(tc_setup_cb_call);