Thu, Jul 26, 2018 at 09:59:30PM CEST, jakub.kicin...@netronome.com wrote:
>On Thu, 26 Jul 2018 18:31:01 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <j...@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 <j...@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,
>
