Re: [PATCH net-next,v2 03/12] flow_dissector: add flow action infrastructure

Jiri Pirko
2018-11-19T23:35:40Z

Mon, Nov 19, 2018 at 01:35:48PM CET, pablo@netfilter.org wrote:
>On Mon, Nov 19, 2018 at 12:56:23PM +0100, Jiri Pirko wrote:
>> Mon, Nov 19, 2018 at 01:15:10AM CET, pablo@netfilter.org wrote:
>> >This new infrastructure defines the nic actions that you can perform
>> >from existing network drivers. This infrastructure allows us to avoid a
>> >direct dependency with the native software TC action representation.
>> >
>> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> >---
>> >v2: no changes.
>> >
>> > include/net/flow_dissector.h | 70 ++++++++++++++++++++++++++++++++++++++++++++
>> > net/core/flow_dissector.c | 18 ++++++++++++
>> > 2 files changed, 88 insertions(+)
>> >
>> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>> >index 965a82b8d881..925c208816f1 100644
>> >--- a/include/net/flow_dissector.h
>> >+++ b/include/net/flow_dissector.h
>> >@@ -402,8 +402,78 @@ void flow_rule_match_enc_keyid(const struct flow_rule *rule,
>> > void flow_rule_match_enc_opts(const struct flow_rule *rule,
>> > struct flow_match_enc_opts *out);
>> >
>> >+enum flow_action_key_id {
>>
>> Why "key"? Why not just "flow_action_id"
>
>Sure, will rename this.
>
>> >+ FLOW_ACTION_KEY_ACCEPT = 0,
>> >+ FLOW_ACTION_KEY_DROP,
>> >+ FLOW_ACTION_KEY_TRAP,
>> >+ FLOW_ACTION_KEY_GOTO,
>> >+ FLOW_ACTION_KEY_REDIRECT,
>> >+ FLOW_ACTION_KEY_MIRRED,
>> >+ FLOW_ACTION_KEY_VLAN_PUSH,
>> >+ FLOW_ACTION_KEY_VLAN_POP,
>> >+ FLOW_ACTION_KEY_VLAN_MANGLE,
>> >+ FLOW_ACTION_KEY_TUNNEL_ENCAP,
>> >+ FLOW_ACTION_KEY_TUNNEL_DECAP,
>> >+ FLOW_ACTION_KEY_MANGLE,
>> >+ FLOW_ACTION_KEY_ADD,
>> >+ FLOW_ACTION_KEY_CSUM,
>> >+ FLOW_ACTION_KEY_MARK,
>
>I assume I should remove _KEY_ from this enum definitions too.

Sure.

>
>> >+};
>> >+
>> >+/* This is mirroring enum pedit_header_type definition for easy mapping between
>> >+ * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
>> >+ * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
>> >+ */
>> >+enum flow_act_mangle_base {
>>
>> Please be consistent in naming: "act" vs "action"
>
>OK.
>
>> >+ FLOW_ACT_MANGLE_UNSPEC = 0,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_ETH,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP4,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_IP6,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_TCP,
>> >+ FLOW_ACT_MANGLE_HDR_TYPE_UDP,
>> >+};
>> >+
>> >+struct flow_action_key {
>>
>> And here "struct flow_action"
>
>OK.
>
>> >+ enum flow_action_key_id id;
>> >+ union {
>> >+ u32 chain_index; /* FLOW_ACTION_KEY_GOTO */
>> >+ struct net_device *dev; /* FLOW_ACTION_KEY_REDIRECT */
>> >+ struct { /* FLOW_ACTION_KEY_VLAN */
>> >+ u16 vid;
>> >+ __be16 proto;
>> >+ u8 prio;
>> >+ } vlan;
>> >+ struct { /* FLOW_ACTION_KEY_PACKET_EDIT */
>> >+ enum flow_act_mangle_base htype;
>> >+ u32 offset;
>> >+ u32 mask;
>> >+ u32 val;
>> >+ } mangle;
>> >+ const struct ip_tunnel_info *tunnel; /* FLOW_ACTION_KEY_TUNNEL_ENCAP */
>> >+ u32 csum_flags; /* FLOW_ACTION_KEY_CSUM */
>> >+ u32 mark; /* FLOW_ACTION_KEY_MARK */
>> >+ };
>> >+};
>> >+
>> >+struct flow_action {
>>
>> And here "struct flow_actions"
>>
>>
>> >+ int num_keys;
>>
>> unsigned int;
>
>OK.