Re: [PATCH net-next 04/12] net: sched: flower: track filter deletion with flag

[Stefano Brivio]

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

> +static int __fl_delete(struct tcf_proto *tp, struct cls_fl_filter *f,
> +                    bool *last, struct netlink_ext_ack *extack)

This would be easier to follow (at least for me):

>  {
>       struct cls_fl_head *head = fl_head_dereference(tp);
>       bool async = tcf_exts_get_net(&f->exts);
> -     bool last;
> -
> -     idr_remove(&head->handle_idr, f->handle);
> -     list_del_rcu(&f->list);
> -     last = fl_mask_put(head, f->mask, async);
> -     if (!tc_skip_hw(f->flags))
> -             fl_hw_destroy_filter(tp, f, extack);
> -     tcf_unbind_filter(tp, &f->res);
> -     __fl_put(f);
> +     int err = 0;

without this

> +
> +     (*last) = false;

with *last = false;

> +
> +     if (!f->deleted) {

with:
        if (f->deleted)
                return -ENOENT;

> +             f->deleted = true;
> +             rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
> +                                    f->mask->filter_ht_params);
> +             idr_remove(&head->handle_idr, f->handle);
> +             list_del_rcu(&f->list);
> +             (*last) = fl_mask_put(head, f->mask, async);

with:
        *last = fl_mask_put(head, f->mask, async);

> +             if (!tc_skip_hw(f->flags))
> +                     fl_hw_destroy_filter(tp, f, extack);
> +             tcf_unbind_filter(tp, &f->res);
> +             __fl_put(f);

and a return 0; here

> +     } else {
> +             err = -ENOENT;
> +     }
>  
> -     return last;
> +     return err;
>  }
>  
> [...]
>
> @@ -1520,14 +1541,14 @@ static int fl_delete(struct tcf_proto *tp, void *arg, 
> bool *last,
>  {
>       struct cls_fl_head *head = fl_head_dereference(tp);
>       struct cls_fl_filter *f = arg;
> +     bool last_on_mask;

This is unused in this series, maybe change __fl_delete() to optionally
take NULL as 'bool *last' argument?

> +     int err = 0;

Nit: no need to initialise this.
 
> -     rhashtable_remove_fast(&f->mask->ht, &f->ht_node,
> -                            f->mask->filter_ht_params);
> -     __fl_delete(tp, f, extack);
> +     err = __fl_delete(tp, f, &last_on_mask, extack);
>       *last = list_empty(&head->masks);
>       __fl_put(f);
>  
> -     return 0;
> +     return err;
>  }
>  
>  static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,

-- 
Stefano