Re: [PATCH net 2/2] net/sched: add delete_empty() to filters and use it in cls_flower

[Vlad Buslov <vladbu@mellanox.com>]

On Tue 24 Dec 2019 at 11:30, Davide Caratti <dcaratti@redhat.com> wrote:
> on tc filters that don't support lockless insertion/removal, there is no
> need to guard against concurrent insertion when a removal is in progress.
> Therefore, we can avoid taking the filter lock and doing a full walk()
> when deleting: it's sufficient to decrease the refcount.
> This fixes situations where walk() was wrongly detecting a non-empty
> filter on deletion, like it happened with cls_u32 in the error path of
> change(), thus leading to failures in the following tdc selftests:
>
>  6aa7: (filter, u32) Add/Replace u32 with source match and invalid indev
>  6658: (filter, u32) Add/Replace u32 with custom hash table and invalid handle
>  74c2: (filter, u32) Add/Replace u32 filter with invalid hash table id
>
> On cls_flower, and on (future) lockless filters, this check is necessary:
> move all the check_empty() logic in a callback so that each filter
> can have its own implementation.
>
> Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
> Suggested-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Suggested-by: Vlad Buslov <vladbu@mellanox.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  include/net/sch_generic.h |  2 ++
>  net/sched/cls_api.c       | 29 ++++-------------------------
>  net/sched/cls_flower.c    | 23 +++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 144f264ea394..5e294da0967e 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -308,6 +308,8 @@ struct tcf_proto_ops {
>  	int			(*delete)(struct tcf_proto *tp, void *arg,
>  					  bool *last, bool rtnl_held,
>  					  struct netlink_ext_ack *);
> +	bool			(*delete_empty)(struct tcf_proto *tp,
> +						bool rtnl_held);

Hi Davide,

Thanks again for fixing this!

Could you add a comment to TCF_PROTO_OPS_DOIT_UNLOCKED flag with
something like "Classifiers implementing this flag are expected to
define tcf_proto_ops->delete_empty(), otherwise hard to debug race
conditions can occur during classifier instance deletion with concurrent
filter insertion."? My original intention was not to require unlocked
classifiers to implement any new APIs but since it is no longer the case
it is better if we document it.

>  	void			(*walk)(struct tcf_proto *tp,
>  					struct tcf_walker *arg, bool rtnl_held);
>  	int			(*reoffload)(struct tcf_proto *tp, bool add,
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 6a0eacafdb19..7900db8d4c06 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -308,33 +308,12 @@ static void tcf_proto_put(struct tcf_proto *tp, bool rtnl_held,
>  		tcf_proto_destroy(tp, rtnl_held, true, extack);
>  }
>
> -static int walker_check_empty(struct tcf_proto *tp, void *fh,
> -			      struct tcf_walker *arg)
> -{
> -	if (fh) {
> -		arg->nonempty = true;
> -		return -1;
> -	}
> -	return 0;
> -}
> -
> -static bool tcf_proto_is_empty(struct tcf_proto *tp, bool rtnl_held)
> -{
> -	struct tcf_walker walker = { .fn = walker_check_empty, };
> -
> -	if (tp->ops->walk) {
> -		tp->ops->walk(tp, &walker, rtnl_held);
> -		return !walker.nonempty;
> -	}
> -	return true;
> -}
> -
>  static bool tcf_proto_check_delete(struct tcf_proto *tp, bool rtnl_held)
>  {
> -	spin_lock(&tp->lock);
> -	if (tcf_proto_is_empty(tp, rtnl_held))
> -		tp->deleting = true;
> -	spin_unlock(&tp->lock);
> +	if (tp->ops->delete_empty)
> +		return tp->ops->delete_empty(tp, rtnl_held);
> +
> +	tp->deleting = true;
>  	return tp->deleting;
>  }
>
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 0d125de54285..e0316d18529e 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -2773,6 +2773,28 @@ static void fl_bind_class(void *fh, u32 classid, unsigned long cl)
>  		f->res.class = cl;
>  }
>
> +static int walker_check_empty(struct tcf_proto *tp, void *fh,
> +			      struct tcf_walker *arg)
> +{
> +	if (fh) {
> +		arg->nonempty = true;
> +		return -1;
> +	}
> +	return 0;
> +}
> +
> +static bool fl_delete_empty(struct tcf_proto *tp, bool rtnl_held)
> +{
> +	struct tcf_walker walker = { .fn = walker_check_empty, };
> +
> +	spin_lock(&tp->lock);
> +	fl_walk(tp, &walker, rtnl_held);
> +	tp->deleting = !walker.nonempty;

I guess we can reduce this code to just:

spin_lock(&tp->lock);
tp->deleting = idr_is_empty(&head->handle_idr);
spin_unlock(&tp->lock);

> +	spin_unlock(&tp->lock);
> +
> +	return tp->deleting;
> +}
> +
>  static struct tcf_proto_ops cls_fl_ops __read_mostly = {
>  	.kind		= "flower",
>  	.classify	= fl_classify,
> @@ -2782,6 +2804,7 @@ static struct tcf_proto_ops cls_fl_ops __read_mostly = {
>  	.put		= fl_put,
>  	.change		= fl_change,
>  	.delete		= fl_delete,
> +	.delete_empty	= fl_delete_empty,
>  	.walk		= fl_walk,
>  	.reoffload	= fl_reoffload,
>  	.hw_add		= fl_hw_add,