| Message ID | 1457016960-27832-4-git-send-email-amir@vadai.me |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Thu, Mar 03, 2016 at 03:55:53PM CET, amir@vadai.me wrote: >Introduce the macros tc_no_actions and tc_for_each_action to make code >clearer. > >Suggested-by: Jiri Pirko <jiri@mellanox.com> >Signed-off-by: Amir Vadai <amir@vadai.me> Acked-by: Jiri Pirko <jiri@mellanox.com>
On Thu, Mar 3, 2016 at 6:55 AM, Amir Vadai <amir@vadai.me> wrote: > Introduce the macros tc_no_actions and tc_for_each_action to make code > clearer. > > Suggested-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Amir Vadai <amir@vadai.me> > --- > include/net/act_api.h | 21 ++++++++++++++++----- > include/net/tc_act/tc_gact.h | 4 ++-- > 2 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/include/net/act_api.h b/include/net/act_api.h > index 342be6c..2a19fe1 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -78,11 +78,6 @@ static inline void tcf_lastuse_update(struct tcf_t *tm) > tm->lastuse = now; > } > > -#ifdef CONFIG_NET_CLS_ACT > - > -#define ACT_P_CREATED 1 > -#define ACT_P_DELETED 1 > - > struct tc_action { > void *priv; > const struct tc_action_ops *ops; > @@ -92,6 +87,11 @@ struct tc_action { > struct tcf_hashinfo *hinfo; > }; You also expose struct tc_action out of CONFIG_NET_CLS_ACT, which you never mention in your changelog at all. So why?
On Thu, Mar 03, 2016 at 09:45:28AM -0800, Cong Wang wrote: > On Thu, Mar 3, 2016 at 6:55 AM, Amir Vadai <amir@vadai.me> wrote: > > Introduce the macros tc_no_actions and tc_for_each_action to make code > > clearer. > > > > Suggested-by: Jiri Pirko <jiri@mellanox.com> > > Signed-off-by: Amir Vadai <amir@vadai.me> > > --- > > include/net/act_api.h | 21 ++++++++++++++++----- > > include/net/tc_act/tc_gact.h | 4 ++-- > > 2 files changed, 18 insertions(+), 7 deletions(-) > > > > diff --git a/include/net/act_api.h b/include/net/act_api.h > > index 342be6c..2a19fe1 100644 > > --- a/include/net/act_api.h > > +++ b/include/net/act_api.h > > @@ -78,11 +78,6 @@ static inline void tcf_lastuse_update(struct tcf_t *tm) > > tm->lastuse = now; > > } > > > > -#ifdef CONFIG_NET_CLS_ACT > > - > > -#define ACT_P_CREATED 1 > > -#define ACT_P_DELETED 1 > > - > > struct tc_action { > > void *priv; > > const struct tc_action_ops *ops; > > @@ -92,6 +87,11 @@ struct tc_action { > > struct tcf_hashinfo *hinfo; > > }; > > You also expose struct tc_action out of CONFIG_NET_CLS_ACT, > which you never mention in your changelog at all. Yes - it was a mistake not to mention it in the changelog. > > So why? The struct will not be used, and without exposing it, the compiler will complain on code like I have in patch 9/10 ("net/mlx5e: Support offload cls_flower with drop action"): static int parse_tc_actions(struct mlx5e_priv *priv, struct tcf_exts *exts, u32 *action, u32 *flow_tag) { const struct tc_action *a; if (tc_no_actions(exts)) return -EINVAL; *flow_tag = MLX5_FS_DEFAULT_FLOW_TAG; *action = 0; tc_for_each_action(a, exts) { [...]
On 16-03-03 06:55 AM, Amir Vadai wrote: > Introduce the macros tc_no_actions and tc_for_each_action to make code > clearer. > > Suggested-by: Jiri Pirko <jiri@mellanox.com> > Signed-off-by: Amir Vadai <amir@vadai.me> > --- Acked-by: John Fastabend <john.r.fastabend@intel.com> This should clean up the driver implementation a bit thanks.
On Thu, Mar 3, 2016 at 11:51 AM, Amir Vadai <amir@vadai.me> wrote: > On Thu, Mar 03, 2016 at 09:45:28AM -0800, Cong Wang wrote: >> >> So why? > The struct will not be used, and without exposing it, the compiler will > complain on code like I have in patch 9/10 ("net/mlx5e: Support offload > cls_flower with drop action"): > > static int parse_tc_actions(struct mlx5e_priv *priv, struct tcf_exts *exts, > u32 *action, u32 *flow_tag) Why not make this a nop when CONFIG_NET_CLS_ACT is not set?
On Fri, Mar 04, 2016 at 10:20:18AM -0800, Cong Wang wrote: > On Thu, Mar 3, 2016 at 11:51 AM, Amir Vadai <amir@vadai.me> wrote: > > On Thu, Mar 03, 2016 at 09:45:28AM -0800, Cong Wang wrote: > >> > >> So why? > > The struct will not be used, and without exposing it, the compiler will > > complain on code like I have in patch 9/10 ("net/mlx5e: Support offload > > cls_flower with drop action"): > > > > static int parse_tc_actions(struct mlx5e_priv *priv, struct tcf_exts *exts, > > u32 *action, u32 *flow_tag) > > Why not make this a nop when CONFIG_NET_CLS_ACT is not set? In V0 I did make it a nop. Jiri has suggested [1] that I will replace the ifdefs with the macro's tc_for_each_action and is_tcf_gact_shot. And I do think it looks more elegant. Why do you think it is a problem to expose truct tc_action? Thanks for your review, Amir [1] - https://patchwork.ozlabs.org/patch/590550/
diff --git a/include/net/act_api.h b/include/net/act_api.h index 342be6c..2a19fe1 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -78,11 +78,6 @@ static inline void tcf_lastuse_update(struct tcf_t *tm) tm->lastuse = now; } -#ifdef CONFIG_NET_CLS_ACT - -#define ACT_P_CREATED 1 -#define ACT_P_DELETED 1 - struct tc_action { void *priv; const struct tc_action_ops *ops; @@ -92,6 +87,11 @@ struct tc_action { struct tcf_hashinfo *hinfo; }; +#ifdef CONFIG_NET_CLS_ACT + +#define ACT_P_CREATED 1 +#define ACT_P_DELETED 1 + struct tc_action_ops { struct list_head head; char kind[IFNAMSIZ]; @@ -171,5 +171,16 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int); int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int); int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int); int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int); + +#define tc_no_actions(_exts) \ + (list_empty(&(_exts)->actions)) + +#define tc_for_each_action(_a, _exts) \ + list_for_each_entry(a, &(_exts)->actions, list) +#else /* CONFIG_NET_CLS_ACT */ + +#define tc_no_actions(_exts) true +#define tc_for_each_action(_a, _exts) while (0) + #endif /* CONFIG_NET_CLS_ACT */ #endif diff --git a/include/net/tc_act/tc_gact.h b/include/net/tc_act/tc_gact.h index 04a3183..93c520b 100644 --- a/include/net/tc_act/tc_gact.h +++ b/include/net/tc_act/tc_gact.h @@ -16,9 +16,9 @@ struct tcf_gact { #define to_gact(a) \ container_of(a->priv, struct tcf_gact, common) -#ifdef CONFIG_NET_CLS_ACT static inline bool is_tcf_gact_shot(const struct tc_action *a) { +#ifdef CONFIG_NET_CLS_ACT struct tcf_gact *gact; if (a->ops && a->ops->type != TCA_ACT_GACT) @@ -28,7 +28,7 @@ static inline bool is_tcf_gact_shot(const struct tc_action *a) if (gact->tcf_action == TC_ACT_SHOT) return true; +#endif return false; } -#endif #endif /* __NET_TC_GACT_H */
Introduce the macros tc_no_actions and tc_for_each_action to make code clearer. Suggested-by: Jiri Pirko <jiri@mellanox.com> Signed-off-by: Amir Vadai <amir@vadai.me> --- include/net/act_api.h | 21 ++++++++++++++++----- include/net/tc_act/tc_gact.h | 4 ++-- 2 files changed, 18 insertions(+), 7 deletions(-)