Re: [PATCH net-next] net: sched: remove redundant 'rtnl_held' argument

[Vlad Buslov <vladbu@nvidia.com>]

On Tue 01 Dec 2020 at 19:03, Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 1 Dec 2020 09:55:37 +0200 Vlad Buslov wrote:
>> On Tue 01 Dec 2020 at 04:52, Jakub Kicinski <kuba@kernel.org> wrote:
>> > On Fri, 27 Nov 2020 17:12:05 +0200 Vlad Buslov wrote:  
>> >> @@ -2262,7 +2260,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>> >>  
>> >>  	if (prio == 0) {
>> >>  		tfilter_notify_chain(net, skb, block, q, parent, n,
>> >> -				     chain, RTM_DELTFILTER, rtnl_held);
>> >> +				     chain, RTM_DELTFILTER);
>> >>  		tcf_chain_flush(chain, rtnl_held);
>> >>  		err = 0;
>> >>  		goto errout;  
>> >
>> > Hum. This looks off.  
>> 
>> Hi Jakub,
>> 
>> Prio==0 means user requests to flush whole chain. In such case rtnl lock
>> is obtained earlier in tc_del_tfilter():
>> 
>> 	/* Take rtnl mutex if flushing whole chain, block is shared (no qdisc
>> 	 * found), qdisc is not unlocked, classifier type is not specified,
>> 	 * classifier is not unlocked.
>> 	 */
>> 	if (!prio ||
>> 	    (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
>> 	    !tcf_proto_is_unlocked(name)) {
>> 		rtnl_held = true;
>> 		rtnl_lock();
>> 	}
>> 
>
> Makes sense, although seems a little fragile. Why not put a true in
> there, in that case?

Because, as I described in commit message, the function will trigger an
assertion if called without rtnl lock, so passing rtnl_held==false
argument makes no sense and is confusing for the reader.

>
> Do you have a larger plan here? The motivation seems a little unclear
> if I'm completely honest. Are you dropping the rtnl_held from all callers 
> of __tcf_get_next_proto() just to save the extra argument / typing?

The plan is to have 'rtnl_held' arg for functions that can be called
without rtnl lock and not have such argument for functions that require
caller to hold rtnl :)

To elaborate further regarding motivation for this patch: some time ago
I received an email asking why I have rtnl_held arg in function that has
ASSERT_RTNL() in one of its dependencies. I re-read the code and
determined that it was a leftover from earlier version and is not needed
in code that was eventually upstreamed. Removing the argument was an
easy decision since Jiri hates those and repeatedly asked me to minimize
usage of such function arguments, so I didn't expect it to be
controversial.

> That's nice but there's also value in the API being consistent.

Cls_api has multiple functions that don't have 'rtnl_held' argument.
Only functions that can work without rtnl lock have it. Why do you
suggest it is inconsistent to remove it here?