Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
Mon, Nov 19, 2018 at 02:21:41PM CET, pa...@netfilter.org wrote:
>On Mon, Nov 19, 2018 at 01:12:51PM +0100, Jiri Pirko wrote:
>> Mon, Nov 19, 2018 at 01:15:11AM CET, pa...@netfilter.org wrote:
>> >@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block,
>> >struct tcf_exts *exts,
>> > }
>> > EXPORT_SYMBOL(tc_setup_cb_call);
>> >
>> >+int tc_setup_flow_action(struct flow_action *flow_action,
>> >+const struct tcf_exts *exts)
>> >+{
>> >+ const struct tc_action *act;
>> >+ int num_acts = 0, i, j, k;
>> >+
>> >+ if (!exts)
>> >+ return 0;
>> >+
>> >+ tcf_exts_for_each_action(i, act, exts) {
>> >+ if (is_tcf_pedit(act))
>> >+ num_acts += tcf_pedit_nkeys(act);
>> >+ else
>> >+ num_acts++;
>> >+ }
>> >+ if (!num_acts)
>> >+ return 0;
>> >+
>> >+ if (flow_action_init(flow_action, num_acts) < 0)
>>
>> This is actually a "alloc" function. And the counterpart is "free".
>
>I can rename it to _alloc() if you prefer.
>
>> How about to allocate the container struct which would have the [0]
>> trick for the array of action?
>
>You mean turn *keys into keys[0] stub in struct flow_action? This is
>embedded into struct tc_cls_flower_offload, I may need to make a
>second look but I think it won't fly.
>
>BTW, side note: I will rename keys to "array" given keys is not
>semantically appropriate as you mentioned, BTW.
What I suggest is this:
struct flow_actions {
unsinged int action_count;
struct flow_action action[0];
};
And then to have
struct flow_actions *flow_actions_alloc(unsigned int action_count)
{
return kzalloc(sizeof(struct flow_actions) + sizeof(struct flow_action)
* action_count, ..);
}
Something like this.
>
>Thanks!
Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
On Mon, Nov 19, 2018 at 01:12:51PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:11AM CET, pa...@netfilter.org wrote:
> >@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct
> >tcf_exts *exts,
> > }
> > EXPORT_SYMBOL(tc_setup_cb_call);
> >
> >+int tc_setup_flow_action(struct flow_action *flow_action,
> >+ const struct tcf_exts *exts)
> >+{
> >+const struct tc_action *act;
> >+int num_acts = 0, i, j, k;
> >+
> >+if (!exts)
> >+return 0;
> >+
> >+tcf_exts_for_each_action(i, act, exts) {
> >+if (is_tcf_pedit(act))
> >+num_acts += tcf_pedit_nkeys(act);
> >+else
> >+num_acts++;
> >+}
> >+if (!num_acts)
> >+return 0;
> >+
> >+if (flow_action_init(flow_action, num_acts) < 0)
>
> This is actually a "alloc" function. And the counterpart is "free".
I can rename it to _alloc() if you prefer.
> How about to allocate the container struct which would have the [0]
> trick for the array of action?
You mean turn *keys into keys[0] stub in struct flow_action? This is
embedded into struct tc_cls_flower_offload, I may need to make a
second look but I think it won't fly.
BTW, side note: I will rename keys to "array" given keys is not
semantically appropriate as you mentioned, BTW.
Thanks!
Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
On Mon, Nov 19, 2018 at 01:16:30PM +0100, Jiri Pirko wrote: > >@@ -391,18 +392,27 @@ static int fl_hw_replace_filter(struct tcf_proto *tp, > > cls_flower.exts = &f->exts; > > cls_flower.classid = f->res.classid; > > > >+if (tc_setup_flow_action(&f->action, &f->exts) < 0) > >+return -ENOMEM; > >+ > >+cls_flower.rule.action.keys = f->action.keys; > >+cls_flower.rule.action.num_keys = f->action.num_keys; > > Hmm, I think flow actions should be only field in rule. Flower does not > use it internally, so it does not really make sense to have f->action OK, will remove this new field from flower. Thanks!
Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
Mon, Nov 19, 2018 at 01:15:11AM CET, pa...@netfilter.org wrote:
>This patch implements a new function to translate from native TC action
>to the new flow_action representation. Moreover, this patch also updates
>cls_flower to use this new function.
>
>Signed-off-by: Pablo Neira Ayuso
>---
>v2: no changes.
>
> include/net/pkt_cls.h | 3 ++
> net/sched/cls_api.c| 113 +
> net/sched/cls_flower.c | 15 ++-
> 3 files changed, 130 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 8b79a1a3a5c7..7d7aefa5fcd2 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -619,6 +619,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> }
> #endif /* CONFIG_NET_CLS_IND */
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts);
>+
> int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
>enum tc_setup_type type, void *type_data, bool err_stop);
>
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index d92f44ac4c39..6ab44e650f43 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -31,6 +31,14 @@
> #include
> #include
> #include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>
> extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
>
>@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct
>tcf_exts *exts,
> }
> EXPORT_SYMBOL(tc_setup_cb_call);
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts)
>+{
>+ const struct tc_action *act;
>+ int num_acts = 0, i, j, k;
>+
>+ if (!exts)
>+ return 0;
>+
>+ tcf_exts_for_each_action(i, act, exts) {
>+ if (is_tcf_pedit(act))
>+ num_acts += tcf_pedit_nkeys(act);
>+ else
>+ num_acts++;
>+ }
>+ if (!num_acts)
>+ return 0;
>+
>+ if (flow_action_init(flow_action, num_acts) < 0)
>+ return -ENOMEM;
>+
>+ j = 0;
>+ tcf_exts_for_each_action(i, act, exts) {
>+ struct flow_action_key *key;
>+
>+ key = &flow_action->keys[j];
>+ if (is_tcf_gact_ok(act)) {
>+ key->id = FLOW_ACTION_KEY_ACCEPT;
>+ } else if (is_tcf_gact_shot(act)) {
>+ key->id = FLOW_ACTION_KEY_DROP;
>+ } else if (is_tcf_gact_trap(act)) {
>+ key->id = FLOW_ACTION_KEY_TRAP;
>+ } else if (is_tcf_gact_goto_chain(act)) {
>+ key->id = FLOW_ACTION_KEY_GOTO;
>+ key->chain_index = tcf_gact_goto_chain_index(act);
>+ } else if (is_tcf_mirred_egress_redirect(act)) {
>+ key->id = FLOW_ACTION_KEY_REDIRECT;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_mirred_egress_mirror(act)) {
>+ key->id = FLOW_ACTION_KEY_MIRRED;
>+ key->dev = tcf_mirred_dev(act);
>+ } else if (is_tcf_vlan(act)) {
>+ switch (tcf_vlan_action(act)) {
>+ case TCA_VLAN_ACT_PUSH:
>+ key->id = FLOW_ACTION_KEY_VLAN_PUSH;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ case TCA_VLAN_ACT_POP:
>+ key->id = FLOW_ACTION_KEY_VLAN_POP;
>+ break;
>+ case TCA_VLAN_ACT_MODIFY:
>+ key->id = FLOW_ACTION_KEY_VLAN_MANGLE;
>+ key->vlan.vid = tcf_vlan_push_vid(act);
>+ key->vlan.proto = tcf_vlan_push_proto(act);
>+ key->vlan.prio = tcf_vlan_push_prio(act);
>+ break;
>+ default:
>+ goto err_out;
>+ }
>+ } else if (is_tcf_tunnel_set(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_ENCAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_tunnel_release(act)) {
>+ key->id = FLOW_ACTION_KEY_TUNNEL_DECAP;
>+ key->tunnel = tcf_tunnel_info(act);
>+ } else if (is_tcf_pedit(act)) {
>+ for (k = 0; k < tcf_pedit_nkeys(act); k++) {
>+ switch (tcf_pedit_cmd(act, k)) {
>+ case TCA_PEDIT_KEY_EX_CMD_SET:
>+ key->id = FLOW_ACTION_KEY_MANGLE;
>+
Re: [PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
Mon, Nov 19, 2018 at 01:15:11AM CET, pa...@netfilter.org wrote:
>This patch implements a new function to translate from native TC action
>to the new flow_action representation. Moreover, this patch also updates
>cls_flower to use this new function.
>
>Signed-off-by: Pablo Neira Ayuso
>---
>v2: no changes.
>
> include/net/pkt_cls.h | 3 ++
> net/sched/cls_api.c| 113 +
> net/sched/cls_flower.c | 15 ++-
> 3 files changed, 130 insertions(+), 1 deletion(-)
>
>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>index 8b79a1a3a5c7..7d7aefa5fcd2 100644
>--- a/include/net/pkt_cls.h
>+++ b/include/net/pkt_cls.h
>@@ -619,6 +619,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> }
> #endif /* CONFIG_NET_CLS_IND */
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts);
>+
> int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
>enum tc_setup_type type, void *type_data, bool err_stop);
>
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index d92f44ac4c39..6ab44e650f43 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -31,6 +31,14 @@
> #include
> #include
> #include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>
> extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
>
>@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct
>tcf_exts *exts,
> }
> EXPORT_SYMBOL(tc_setup_cb_call);
>
>+int tc_setup_flow_action(struct flow_action *flow_action,
>+ const struct tcf_exts *exts)
>+{
>+ const struct tc_action *act;
>+ int num_acts = 0, i, j, k;
>+
>+ if (!exts)
>+ return 0;
>+
>+ tcf_exts_for_each_action(i, act, exts) {
>+ if (is_tcf_pedit(act))
>+ num_acts += tcf_pedit_nkeys(act);
>+ else
>+ num_acts++;
>+ }
>+ if (!num_acts)
>+ return 0;
>+
>+ if (flow_action_init(flow_action, num_acts) < 0)
This is actually a "alloc" function. And the counterpart is "free".
How about to allocate the container struct which would have the [0]
trick for the array of action?
[...]
[PATCH net-next,v2 04/12] cls_api: add translator to flow_action representation
This patch implements a new function to translate from native TC action
to the new flow_action representation. Moreover, this patch also updates
cls_flower to use this new function.
Signed-off-by: Pablo Neira Ayuso
---
v2: no changes.
include/net/pkt_cls.h | 3 ++
net/sched/cls_api.c| 113 +
net/sched/cls_flower.c | 15 ++-
3 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 8b79a1a3a5c7..7d7aefa5fcd2 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -619,6 +619,9 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
}
#endif /* CONFIG_NET_CLS_IND */
+int tc_setup_flow_action(struct flow_action *flow_action,
+const struct tcf_exts *exts);
+
int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
enum tc_setup_type type, void *type_data, bool err_stop);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d92f44ac4c39..6ab44e650f43 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -31,6 +31,14 @@
#include
#include
#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
@@ -2567,6 +2575,111 @@ int tc_setup_cb_call(struct tcf_block *block, struct
tcf_exts *exts,
}
EXPORT_SYMBOL(tc_setup_cb_call);
+int tc_setup_flow_action(struct flow_action *flow_action,
+const struct tcf_exts *exts)
+{
+ const struct tc_action *act;
+ int num_acts = 0, i, j, k;
+
+ if (!exts)
+ return 0;
+
+ tcf_exts_for_each_action(i, act, exts) {
+ if (is_tcf_pedit(act))
+ num_acts += tcf_pedit_nkeys(act);
+ else
+ num_acts++;
+ }
+ if (!num_acts)
+ return 0;
+
+ if (flow_action_init(flow_action, num_acts) < 0)
+ return -ENOMEM;
+
+ j = 0;
+ tcf_exts_for_each_action(i, act, exts) {
+ struct flow_action_key *key;
+
+ key = &flow_action->keys[j];
+ if (is_tcf_gact_ok(act)) {
+ key->id = FLOW_ACTION_KEY_ACCEPT;
+ } else if (is_tcf_gact_shot(act)) {
+ key->id = FLOW_ACTION_KEY_DROP;
+ } else if (is_tcf_gact_trap(act)) {
+ key->id = FLOW_ACTION_KEY_TRAP;
+ } else if (is_tcf_gact_goto_chain(act)) {
+ key->id = FLOW_ACTION_KEY_GOTO;
+ key->chain_index = tcf_gact_goto_chain_index(act);
+ } else if (is_tcf_mirred_egress_redirect(act)) {
+ key->id = FLOW_ACTION_KEY_REDIRECT;
+ key->dev = tcf_mirred_dev(act);
+ } else if (is_tcf_mirred_egress_mirror(act)) {
+ key->id = FLOW_ACTION_KEY_MIRRED;
+ key->dev = tcf_mirred_dev(act);
+ } else if (is_tcf_vlan(act)) {
+ switch (tcf_vlan_action(act)) {
+ case TCA_VLAN_ACT_PUSH:
+ key->id = FLOW_ACTION_KEY_VLAN_PUSH;
+ key->vlan.vid = tcf_vlan_push_vid(act);
+ key->vlan.proto = tcf_vlan_push_proto(act);
+ key->vlan.prio = tcf_vlan_push_prio(act);
+ break;
+ case TCA_VLAN_ACT_POP:
+ key->id = FLOW_ACTION_KEY_VLAN_POP;
+ break;
+ case TCA_VLAN_ACT_MODIFY:
+ key->id = FLOW_ACTION_KEY_VLAN_MANGLE;
+ key->vlan.vid = tcf_vlan_push_vid(act);
+ key->vlan.proto = tcf_vlan_push_proto(act);
+ key->vlan.prio = tcf_vlan_push_prio(act);
+ break;
+ default:
+ goto err_out;
+ }
+ } else if (is_tcf_tunnel_set(act)) {
+ key->id = FLOW_ACTION_KEY_TUNNEL_ENCAP;
+ key->tunnel = tcf_tunnel_info(act);
+ } else if (is_tcf_tunnel_release(act)) {
+ key->id = FLOW_ACTION_KEY_TUNNEL_DECAP;
+ key->tunnel = tcf_tunnel_info(act);
+ } else if (is_tcf_pedit(act)) {
+ for (k = 0; k < tcf_pedit_nkeys(act); k++) {
+ switch (tcf_pedit_cmd(act, k)) {
+ case TCA_PEDIT_KEY_EX_CMD_SET:
+ key->id = FLOW_ACTION_KEY_MANGLE;
+ break;
+ case TCA_PEDIT_KEY_EX_CMD_ADD:
+
