On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vla...@mellanox.com> wrote:
>
> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> at least once is unreliable. Some classifiers set 'stop' flag on error
> before calling walker callback, other classifiers used to call it with NULL
> filter pointer when empty. In order to prevent further regressions, extend
> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> tcf_walker->fn() implementation that is used to check if classifier has
> filters configured.
So, after this patch commits like 31a998487641 ("net: sched: fw: don't
set arg->stop in fw_walk() when empty") can be reverted??
>
> Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent
> execution")
> Signed-off-by: Vlad Buslov <vla...@mellanox.com>
> Suggested-by: Cong Wang <xiyou.wangc...@gmail.com>
> ---
> include/net/pkt_cls.h | 1 +
> net/sched/cls_api.c | 13 +++++++++----
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 232f801f2a21..422dd8800478 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -17,6 +17,7 @@ struct tcf_walker {
> int stop;
> int skip;
> int count;
> + bool nonempty;
> unsigned long cookie;
> int (*fn)(struct tcf_proto *, void *node, struct tcf_walker *);
> };
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index e2c888961379..3543be31d400 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -238,18 +238,23 @@ static void tcf_proto_put(struct tcf_proto *tp, bool
> rtnl_held,
> tcf_proto_destroy(tp, rtnl_held, extack);
> }
>
> -static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
> +static int walker_check_empty(struct tcf_proto *tp, void *d,
> + struct tcf_walker *arg)
> {
> - return -1;
> + if (tp) {
> + arg->nonempty = true;
> + return -1;
> + }
> + return 0;
How does this even work? If we can simply check tp!=NULL as
non-empty, why do we even need a walker??
For me, it must be pushed down to each implementation to
determine how it is empty.
