| Message ID | CAM_iQpXzXHc7o1WOtkNnF+io_WZFP+DP4zWm8PZbwX7SC38=Pw@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On 09/28/2014 10:15 AM, Cong Wang wrote: > On Sun, Sep 28, 2014 at 8:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote: >> On 09/26/14 19:25, John Fastabend wrote: >>> >>> On 09/26/2014 03:48 PM, Jamal Hadi Salim wrote: >> >> >>>> Hrm - poking inside there and this seems to be one of those >>>> classifiers that hasnt been getting a lot of love. Doesnt >>>> support actions well for example. >>>> >>>> It seems to work for me: >>> >>> >>> This works for me as well try adding two or more policers and >>> check if they all get printed. >>> >> >> Multiple actions wont work for this classifier. I have time, >> will send patch. >> > > Hmm, looks like there is a bug in police dump. Please > try the attached patch. > > Thanks. > I don't think we need this change, (or perhaps it needs to be a bit more complete if something is missing) take a look a tcf_exts_validate(), notice the policer is added to act->list so the if block in dump() work out, int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, struct nlattr *rate_tlv, struct tcf_exts *exts, bool ovr) { #ifdef CONFIG_NET_CLS_ACT { struct tc_action *act; INIT_LIST_HEAD(&exts->actions); if (exts->police && tb[exts->police]) { act = tcf_action_init_1(net, tb[exts->police], rate_tlv, "police", ovr, TCA_ACT_BIND); if (IS_ERR(act)) return PTR_ERR(act); act->type = exts->type = TCA_OLD_COMPAT; list_add(&act->list, &exts->actions); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } else if (exts->action && tb[exts->action]) { int err; err = tcf_action_init(net, tb[exts->action], rate_tlv, NULL, ovr, TCA_ACT_BIND, &exts->actions); if (err) return err; } } [...]
On 09/28/14 13:50, John Fastabend wrote: > On 09/28/2014 10:15 AM, Cong Wang wrote: > act->type = exts->type = TCA_OLD_COMPAT; > list_add(&act->list, &exts->actions); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Right. Note, this exts->xx thing we should eventually kill. It was intended to allow for old binary compatibility (for about 10 years now). In the earlier days, policer was a speacial action and one could invoke it directly from "tc filter ....." But over time, the more sane way is: "tc filter .. action police ..." We have killed ability to compile in the kernel policer to support to allow old tools to continue to use it at least 5 years ago. tc still has support but I doubt if anyone uses it. Otherwise there would have been a lot of fallout from commit 4bfb21ca2043dc5251102e4debcc0ae27f7304e7 in iproute2. So if there are any takers for patches, i think the first thing is to kill every call in iproute2 which calls police directly. And if nobody whines, then get rid of most of exts->XXX (at minimal there would be no need for exts->police) cheers, jamal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index c28b0d3..1218900 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -576,16 +576,18 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts) if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0) goto nla_put_failure; nla_nest_end(skb, nest); - } else if (exts->police) { - struct tc_action *act = tcf_exts_first_act(exts); - nest = nla_nest_start(skb, exts->police); - if (nest == NULL || !act) - goto nla_put_failure; - if (tcf_action_dump_old(skb, act, 0, 0) < 0) - goto nla_put_failure; - nla_nest_end(skb, nest); } } + + if (exts->police) { + struct tc_action *act = tcf_exts_first_act(exts); + nest = nla_nest_start(skb, exts->police); + if (nest == NULL || !act) + goto nla_put_failure; + if (tcf_action_dump_old(skb, act, 0, 0) < 0) + goto nla_put_failure; + nla_nest_end(skb, nest); + } return 0; nla_put_failure: