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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 14 Feb 2019 09:47:03 +0200
Vlad Buslov <vladbu@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