| Message ID | 20170728144042.6380-5-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
I am probably missing something. All those changes to just replace "if (exts->nr_actions)" with "if (tcf_exts_has_actions(exts))" ? cheers, jamal On 17-07-28 10:40 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Use the tcf_exts_has_actions helper instead or directly testing > exts->nr_actions in tcf_exts_exec. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > include/net/pkt_cls.h | 46 +++++++++++++++++++++++----------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h > index 7f25636..322a282 100644 > --- a/include/net/pkt_cls.h > +++ b/include/net/pkt_cls.h > @@ -177,29 +177,6 @@ tcf_exts_stats_update(const struct tcf_exts *exts, > } > > /** > - * tcf_exts_exec - execute tc filter extensions > - * @skb: socket buffer > - * @exts: tc filter extensions handle > - * @res: desired result > - * > - * Executes all configured extensions. Returns 0 on a normal execution, > - * a negative number if the filter must be considered unmatched or > - * a positive action code (TC_ACT_*) which must be returned to the > - * underlying layer. > - */ > -static inline int > -tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, > - struct tcf_result *res) > -{ > -#ifdef CONFIG_NET_CLS_ACT > - if (exts->nr_actions) > - return tcf_action_exec(skb, exts->actions, exts->nr_actions, > - res); > -#endif > - return 0; > -} > - > -/** > * tcf_exts_has_actions - check if at least one action is present > * @exts: tc filter extensions handle > * > @@ -229,6 +206,29 @@ static inline bool tcf_exts_has_one_action(struct tcf_exts *exts) > #endif > } > > +/** > + * tcf_exts_exec - execute tc filter extensions > + * @skb: socket buffer > + * @exts: tc filter extensions handle > + * @res: desired result > + * > + * Executes all configured extensions. Returns 0 on a normal execution, > + * a negative number if the filter must be considered unmatched or > + * a positive action code (TC_ACT_*) which must be returned to the > + * underlying layer. > + */ > +static inline int > +tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, > + struct tcf_result *res) > +{ > +#ifdef CONFIG_NET_CLS_ACT > + if (tcf_exts_has_actions(exts)) > + return tcf_action_exec(skb, exts->actions, exts->nr_actions, > + res); > +#endif > + return 0; > +} > + > int tcf_exts_validate(struct net *net, struct tcf_proto *tp, > struct nlattr **tb, struct nlattr *rate_tlv, > struct tcf_exts *exts, bool ovr); >
Sun, Jul 30, 2017 at 09:48:24PM CEST, jhs@mojatatu.com wrote: >I am probably missing something. All those changes to just >replace "if (exts->nr_actions)" with "if (tcf_exts_has_actions(exts))" ? That is what the description says :) > >cheers, >jamal > >On 17-07-28 10:40 AM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Use the tcf_exts_has_actions helper instead or directly testing >> exts->nr_actions in tcf_exts_exec. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
On 17-07-31 02:36 AM, Jiri Pirko wrote: > Sun, Jul 30, 2017 at 09:48:24PM CEST, jhs@mojatatu.com wrote: >> I am probably missing something. All those changes to just >> replace "if (exts->nr_actions)" with "if (tcf_exts_has_actions(exts))" ? > > That is what the description says :) > I meant I wouldve expected few liners added/removed. cheers, jamal
Mon, Jul 31, 2017 at 02:09:22PM CEST, jhs@mojatatu.com wrote: >On 17-07-31 02:36 AM, Jiri Pirko wrote: >> Sun, Jul 30, 2017 at 09:48:24PM CEST, jhs@mojatatu.com wrote: >> > I am probably missing something. All those changes to just >> > replace "if (exts->nr_actions)" with "if (tcf_exts_has_actions(exts))" ? >> >> That is what the description says :) >> > >I meant I wouldve expected few liners added/removed. one actually. But the function tcf_exts_exec had to be moved since tcf_exts_has_actions was defined after originaly > >cheers, >jamal
On Fri, Jul 28, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: > +static inline int > +tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, > + struct tcf_result *res) > +{ > +#ifdef CONFIG_NET_CLS_ACT > + if (tcf_exts_has_actions(exts)) > + return tcf_action_exec(skb, exts->actions, exts->nr_actions, > + res); > +#endif > + return 0; > +} While you are on it, can we get rid of this macro too? tcf_action_exec() is only defined with CONFIG_NET_CLS_ACT, not sure if compiler is kind enough to eliminate the false branch for us: if (false) return tcf_action_exec(...); // not defined but the branch is dead At least you can add a wrapper for tcf_action_exec() to just return 0.
Mon, Jul 31, 2017 at 10:37:21PM CEST, xiyou.wangcong@gmail.com wrote: >On Fri, Jul 28, 2017 at 7:40 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> +static inline int >> +tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, >> + struct tcf_result *res) >> +{ >> +#ifdef CONFIG_NET_CLS_ACT >> + if (tcf_exts_has_actions(exts)) >> + return tcf_action_exec(skb, exts->actions, exts->nr_actions, >> + res); >> +#endif >> + return 0; >> +} > > >While you are on it, can we get rid of this macro too? > >tcf_action_exec() is only defined with CONFIG_NET_CLS_ACT, >not sure if compiler is kind enough to eliminate the false branch >for us: > >if (false) > return tcf_action_exec(...); // not defined but the branch is dead > >At least you can add a wrapper for tcf_action_exec() to just >return 0. Did you see? net: sched: remove check for number of actions in tcf_exts_exec I will add static inline stub for tcf_action_exec in case CONFIG_NET_CLS_ACT is not set.
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 7f25636..322a282 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -177,29 +177,6 @@ tcf_exts_stats_update(const struct tcf_exts *exts, } /** - * tcf_exts_exec - execute tc filter extensions - * @skb: socket buffer - * @exts: tc filter extensions handle - * @res: desired result - * - * Executes all configured extensions. Returns 0 on a normal execution, - * a negative number if the filter must be considered unmatched or - * a positive action code (TC_ACT_*) which must be returned to the - * underlying layer. - */ -static inline int -tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, - struct tcf_result *res) -{ -#ifdef CONFIG_NET_CLS_ACT - if (exts->nr_actions) - return tcf_action_exec(skb, exts->actions, exts->nr_actions, - res); -#endif - return 0; -} - -/** * tcf_exts_has_actions - check if at least one action is present * @exts: tc filter extensions handle * @@ -229,6 +206,29 @@ static inline bool tcf_exts_has_one_action(struct tcf_exts *exts) #endif } +/** + * tcf_exts_exec - execute tc filter extensions + * @skb: socket buffer + * @exts: tc filter extensions handle + * @res: desired result + * + * Executes all configured extensions. Returns 0 on a normal execution, + * a negative number if the filter must be considered unmatched or + * a positive action code (TC_ACT_*) which must be returned to the + * underlying layer. + */ +static inline int +tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts, + struct tcf_result *res) +{ +#ifdef CONFIG_NET_CLS_ACT + if (tcf_exts_has_actions(exts)) + return tcf_action_exec(skb, exts->actions, exts->nr_actions, + res); +#endif + return 0; +} + int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb, struct nlattr *rate_tlv, struct tcf_exts *exts, bool ovr);