Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Cong Wang <hidden>
Date: 2019-02-25 22:53:04
On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov [off-list ref] 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??
quoted hunk ↗ jump to hunk
Fixes: 8b64678e0af8 ("net: sched: refactor tp insert/delete for concurrent execution") Signed-off-by: Vlad Buslov <redacted> Suggested-by: Cong Wang <redacted> --- 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.