Re: [patch net-next RFC] net: sched: don't dump chains only held by actions

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jul 26, 2018 at 09:59:30PM CEST, jakub.kicinski@netronome.com wrote:
>On Thu, 26 Jul 2018 18:31:01 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> In case a chain is empty and not explicitly created by a user,
>> such chain should not exist. The only exception is if there is
>> an action "goto chain" pointing to it. In that case, don't show the
>> chain in the dump. Track the chain references held by actions and
>> use them to find out if a chain should or should not be shown
>> in chain dump.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>I don't have any better ideas :)
>
>One question below.
>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 75cce2819de9..76035cd6e3bf 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -262,6 +262,25 @@ static void tcf_chain_hold(struct tcf_chain *chain)
>>  	++chain->refcnt;
>>  }
>>  
>> +static void tcf_chain_hold_by_act(struct tcf_chain *chain)
>> +{
>> +	++chain->action_refcnt;
>> +}
>> +
>> +static void tcf_chain_release_by_act(struct tcf_chain *chain)
>> +{
>> +	--chain->action_refcnt;
>> +}
>> +
>> +static bool tcf_chain_is_zombie(struct tcf_chain *chain)
>> +{
>> +	/* In case all the references are action references, this
>> +	 * chain is a zombie and should not be listed in the chain
>> +	 * dump list.
>> +	 */
>> +	return chain->refcnt == chain->action_refcnt;
>> +}
>> +
>>  static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block,
>>  					  u32 chain_index)
>>  {
>> @@ -298,6 +317,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>  }
>>  EXPORT_SYMBOL(tcf_chain_get);
>>  
>> +struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index)
>> +{
>> +	struct tcf_chain *chain = tcf_chain_get(block, chain_index, true);
>> +
>> +	tcf_chain_hold_by_act(chain);
>> +	return chain;
>> +}
>> +EXPORT_SYMBOL(tcf_chain_get_by_act);
>> +
>>  static void tc_chain_tmplt_del(struct tcf_chain *chain);
>>  
>>  void tcf_chain_put(struct tcf_chain *chain)
>> @@ -310,6 +338,13 @@ void tcf_chain_put(struct tcf_chain *chain)
>>  }
>>  EXPORT_SYMBOL(tcf_chain_put);
>>  
>> +void tcf_chain_put_by_act(struct tcf_chain *chain)
>> +{
>> +	tcf_chain_release_by_act(chain);
>> +	tcf_chain_put(chain);
>> +}
>> +EXPORT_SYMBOL(tcf_chain_put_by_act);
>> +
>>  static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
>>  {
>>  	if (chain->explicitly_created)
>> @@ -1803,17 +1838,26 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
>>  	chain = tcf_chain_lookup(block, chain_index);
>>  	if (n->nlmsg_type == RTM_NEWCHAIN) {
>>  		if (chain) {
>> -			NL_SET_ERR_MSG(extack, "Filter chain already exists");
>> -			return -EEXIST;
>> -		}
>> -		if (!(n->nlmsg_flags & NLM_F_CREATE)) {
>> -			NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
>> -			return -ENOENT;
>> -		}
>> -		chain = tcf_chain_create(block, chain_index);
>> -		if (!chain) {
>> -			NL_SET_ERR_MSG(extack, "Failed to create filter chain");
>> -			return -ENOMEM;
>> +			if (tcf_chain_is_zombie(chain)) {
>> +				/* The chain exists only because there is
>> +				 * some action referencing it, meaning it
>> +				 * is a zombie.
>> +				 */
>> +				tcf_chain_hold(chain);
>
>I'm not 100% sure why this is needed?  In my tree below I see:
>
>	switch (n->nlmsg_type) {
>	case RTM_NEWCHAIN:
>		err = tc_chain_tmplt_add(chain, net, tca, extack);
>		if (err)
>			goto errout;
>		/* In case the chain was successfully added, take a reference
>		 * to the chain. This ensures that an empty chain
>		 * does not disappear at the end of this function.
>		 */
>		tcf_chain_hold(chain);
>		chain->explicitly_created = true;
>
>so one reference will be taken..  do we need two? 

There is a put at the end of this function.

>
>> +			} else {
>> +				NL_SET_ERR_MSG(extack, "Filter chain already exists");
>> +				return -EEXIST;
>> +			}
>> +		} else {
>> +			if (!(n->nlmsg_flags & NLM_F_CREATE)) {
>> +				NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
>> +				return -ENOENT;
>> +			}
>> +			chain = tcf_chain_create(block, chain_index);
>> +			if (!chain) {
>> +				NL_SET_ERR_MSG(extack, "Failed to create filter chain");
>> +				return -ENOMEM;
>> +			}
>>  		}
>>  	} else {
>>  		if (!chain) {
>> @@ -1944,6 +1988,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>>  			index++;
>>  			continue;
>>  		}
>> +		if (tcf_chain_is_zombie(chain))
>> +			continue;
>>  		err = tc_chain_fill_node(chain, net, skb, block,
>>  					 NETLINK_CB(cb->skb).portid,
>>  					 cb->nlh->nlmsg_seq, NLM_F_MULTI,
>