Re: [PATCH net-next 03/12] net: sched: flower: introduce reference counting for filters

[Stefano Brivio]

On Thu, 14 Feb 2019 09:47:03 +0200
Vlad Buslov <vla...@mellanox.com> wrote:

> +static struct cls_fl_filter *fl_get_next_filter(struct tcf_proto *tp,
> +                                             unsigned long *handle)
> +{
> +     struct cls_fl_head *head = fl_head_dereference(tp);
> +     struct cls_fl_filter *f;
> +
> +     rcu_read_lock();
> +     /* don't return filters that are being deleted */
> +     while ((f = idr_get_next_ul(&head->handle_idr,
> +                                 handle)) != NULL &&
> +            !refcount_inc_not_zero(&f->refcnt))
> +             ++(*handle);

This... hurts :) What about:

        while ((f = idr_get_next_ul(&head->handle_idr, &handle))) {
                if (refcount_inc_not_zero(&f->refcnt))
                        break;
                ++(*handle);
        }

?

> +     rcu_read_unlock();
> +
> +     return f;
> +}
> +
>  static bool __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
>                       struct netlink_ext_ack *extack)
>  {
> @@ -456,10 +503,7 @@ static bool __fl_delete(struct tcf_proto *tp, struct 
> cls_fl_filter *f,
>       if (!tc_skip_hw(f->flags))
>               fl_hw_destroy_filter(tp, f, extack);
>       tcf_unbind_filter(tp, &f->res);
> -     if (async)
> -             tcf_queue_work(&f->rwork, fl_destroy_filter_work);
> -     else
> -             __fl_destroy_filter(f);
> +     __fl_put(f);
>  
>       return last;
>  }
> @@ -494,11 +538,18 @@ static void fl_destroy(struct tcf_proto *tp, bool 
> rtnl_held,
>       tcf_queue_work(&head->rwork, fl_destroy_sleepable);
>  }
>  
> +static void fl_put(struct tcf_proto *tp, void *arg)
> +{
> +     struct cls_fl_filter *f = arg;
> +
> +     __fl_put(f);
> +}
> +
>  static void *fl_get(struct tcf_proto *tp, u32 handle)
>  {
>       struct cls_fl_head *head = fl_head_dereference(tp);
>  
> -     return idr_find(&head->handle_idr, handle);
> +     return __fl_get(head, handle);
>  }
>  
>  static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
> @@ -1321,12 +1372,16 @@ static int fl_change(struct net *net, struct sk_buff 
> *in_skb,
>       struct nlattr **tb;
>       int err;
>  
> -     if (!tca[TCA_OPTIONS])
> -             return -EINVAL;
> +     if (!tca[TCA_OPTIONS]) {
> +             err = -EINVAL;
> +             goto errout_fold;
> +     }
>  
>       mask = kzalloc(sizeof(struct fl_flow_mask), GFP_KERNEL);
> -     if (!mask)
> -             return -ENOBUFS;
> +     if (!mask) {
> +             err = -ENOBUFS;
> +             goto errout_fold;
> +     }
>  
>       tb = kcalloc(TCA_FLOWER_MAX + 1, sizeof(struct nlattr *), GFP_KERNEL);
>       if (!tb) {
> @@ -1349,6 +1404,7 @@ static int fl_change(struct net *net, struct sk_buff 
> *in_skb,
>               err = -ENOBUFS;
>               goto errout_tb;
>       }
> +     refcount_set(&fnew->refcnt, 1);
>  
>       err = tcf_exts_init(&fnew->exts, TCA_FLOWER_ACT, 0);
>       if (err < 0)
> @@ -1381,6 +1437,7 @@ static int fl_change(struct net *net, struct sk_buff 
> *in_skb,
>       if (!tc_in_hw(fnew->flags))
>               fnew->flags |= TCA_CLS_FLAGS_NOT_IN_HW;
>  
> +     refcount_inc(&fnew->refcnt);

I guess I'm not getting the semantics but... why is it 2 now?

-- 
Stefano