On Thu, 14 Feb 2019 09:47:02 +0200
Vlad Buslov <vla...@mellanox.com> wrote:
> As a preparation for using classifier spinlock instead of relying on
> external rtnl lock, rearrange code in fl_change. The goal is to group the
> code which changes classifier state in single block in order to allow
> following commits in this set to protect it from parallel modification with
> tp->lock. Data structures that require tp->lock protection are mask
> hashtable and filters list, and classifier handle_idr.
>
> fl_hw_replace_filter() is a sleeping function and cannot be called while
> holding a spinlock. In order to execute all sequence of changes to shared
> classifier data structures atomically, call fl_hw_replace_filter() before
> modifying them.
>
> Signed-off-by: Vlad Buslov <vla...@mellanox.com>
> Acked-by: Jiri Pirko <j...@mellanox.com>
> ---
> net/sched/cls_flower.c | 85
> ++++++++++++++++++++++++++------------------------
> 1 file changed, 44 insertions(+), 41 deletions(-)
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 88d7af78ba7e..91596a6271f8 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -1354,90 +1354,93 @@ static int fl_change(struct net *net, struct sk_buff
> *in_skb,
> if (err < 0)
> goto errout;
>
> - if (!handle) {
> - handle = 1;
> - err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
> - INT_MAX, GFP_KERNEL);
> - } else if (!fold) {
> - /* user specifies a handle and it doesn't exist */
> - err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
> - handle, GFP_KERNEL);
> - }
> - if (err)
> - goto errout;
> - fnew->handle = handle;
> -
>
> [...]
>
> if (fold) {
> + fnew->handle = handle;
I'm probably missing something, but what if fold is passed and the
handle isn't specified? That can still happen, right? In that case we
wouldn't be allocating the handle.
> +
> + err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
> + fnew->mask->filter_ht_params);
> + if (err)
> + goto errout_hw;
> +
> rhashtable_remove_fast(&fold->mask->ht,
> &fold->ht_node,
> fold->mask->filter_ht_params);
> - if (!tc_skip_hw(fold->flags))
> - fl_hw_destroy_filter(tp, fold, NULL);
> - }
> -
> - *arg = fnew;
> -
> - if (fold) {
> idr_replace(&head->handle_idr, fnew, fnew->handle);
> list_replace_rcu(&fold->list, &fnew->list);
> +
> + if (!tc_skip_hw(fold->flags))
> + fl_hw_destroy_filter(tp, fold, NULL);
> tcf_unbind_filter(tp, &fold->res);
> tcf_exts_get_net(&fold->exts);
> tcf_queue_work(&fold->rwork, fl_destroy_filter_work);
> } else {
> + if (__fl_lookup(fnew->mask, &fnew->mkey)) {
> + err = -EEXIST;
> + goto errout_hw;
> + }
> +
> + if (handle) {
> + /* user specifies a handle and it doesn't exist */
> + err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
> + handle, GFP_ATOMIC);
> + } else {
> + handle = 1;
> + err = idr_alloc_u32(&head->handle_idr, fnew, &handle,
> + INT_MAX, GFP_ATOMIC);
> + }
> + if (err)
> + goto errout_hw;
Just if you respin: a newline here would be nice to have.
> + fnew->handle = handle;
> +
> + err = rhashtable_insert_fast(&fnew->mask->ht, &fnew->ht_node,
> + fnew->mask->filter_ht_params);
> + if (err)
> + goto errout_idr;
> +
> list_add_tail_rcu(&fnew->list, &fnew->mask->filters);
> }
>
> + *arg = fnew;
> +
> kfree(tb);
> kfree(mask);
> return 0;
>
> -errout_mask_ht:
> - rhashtable_remove_fast(&fnew->mask->ht, &fnew->ht_node,
> - fnew->mask->filter_ht_params);
> -
> -errout_mask:
> - fl_mask_put(head, fnew->mask, false);
> -
> errout_idr:
> if (!fold)
This check could go away, I guess (not a strong preference though).
> idr_remove(&head->handle_idr, fnew->handle);
> +errout_hw:
> + if (!tc_skip_hw(fnew->flags))
> + fl_hw_destroy_filter(tp, fnew, NULL);
> +errout_mask:
> + fl_mask_put(head, fnew->mask, false);
> errout:
> tcf_exts_destroy(&fnew->exts);
> kfree(fnew);
--
Stefano
