| Message ID | 1584689657-17280-1-git-send-email-wenxu@ucloud.cn |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable | expand |
On Fri, Mar 20, 2020 at 03:34:17PM +0800, wenxu@ucloud.cn wrote: [...] > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c > index ad54931..60289a6 100644 > --- a/net/netfilter/nf_flow_table_offload.c > +++ b/net/netfilter/nf_flow_table_offload.c > @@ -165,8 +165,16 @@ static void flow_offload_mangle(struct flow_action_entry *entry, > flow_action_entry_next(struct nf_flow_rule *flow_rule) > { > int i = flow_rule->rule->action.num_entries++; > + struct flow_action_entry *entry; > + > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_ANY != FLOW_ACTION_HW_STATS_ANY); > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); > + > + entry = &flow_rule->rule->action.entries[i]; > + entry->hw_stats_type = flow_rule->hw_stats_type; Please, use FLOW_ACTION_HW_STATS_ANY by now. Remove the uapi code, from this patch. I'm not sure how users will be using this new knob. At this stage, I think the drivers knows much better what type to pick than the user. You also have to explain me how you are testing things. Thank you.
在 2020/3/20 20:10, Pablo Neira Ayuso 写道: > On Fri, Mar 20, 2020 at 03:34:17PM +0800, wenxu@ucloud.cn wrote: > [...] >> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c >> index ad54931..60289a6 100644 >> --- a/net/netfilter/nf_flow_table_offload.c >> +++ b/net/netfilter/nf_flow_table_offload.c >> @@ -165,8 +165,16 @@ static void flow_offload_mangle(struct flow_action_entry *entry, >> flow_action_entry_next(struct nf_flow_rule *flow_rule) >> { >> int i = flow_rule->rule->action.num_entries++; >> + struct flow_action_entry *entry; >> + >> + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_ANY != FLOW_ACTION_HW_STATS_ANY); >> + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); >> + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); >> + >> + entry = &flow_rule->rule->action.entries[i]; >> + entry->hw_stats_type = flow_rule->hw_stats_type; > Please, use FLOW_ACTION_HW_STATS_ANY by now. Remove the uapi code, > from this patch. > > I'm not sure how users will be using this new knob. > > At this stage, I think the drivers knows much better what type to pick > than the user. Yes, I agree. > > You also have to explain me how you are testing things. I test the flowtable offload with mlnx driver. ALL the flow add in driver failed for checking the hw_stats_type of flow action. static int parse_tc_fdb_actions(struct mlx5e_priv *priv, struct flow_action *flow_action, struct mlx5e_tc_flow *flow, struct netlink_ext_ack *extack) { ................ if (!flow_action_hw_stats_check(flow_action, extack, FLOW_ACTION_HW_STATS_DELAYED_BIT)) return -EOPNOTSUPP; Indeed always set the hw_stats_type of flow_action to FLOW_ACTION_HW_STATS_ANY can fix this. But maybe it can provide a knob for user? Curently with this patch the user don't need set any value with default value FLOW_ACTION_HW_STATS in the kernel. > > Thank you. >
On Fri, Mar 20, 2020 at 08:36:17PM +0800, wenxu wrote: > > 在 2020/3/20 20:10, Pablo Neira Ayuso 写道: > > On Fri, Mar 20, 2020 at 03:34:17PM +0800, wenxu@ucloud.cn wrote: > > [...] > > > diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c > > > index ad54931..60289a6 100644 > > > --- a/net/netfilter/nf_flow_table_offload.c > > > +++ b/net/netfilter/nf_flow_table_offload.c > > > @@ -165,8 +165,16 @@ static void flow_offload_mangle(struct flow_action_entry *entry, > > > flow_action_entry_next(struct nf_flow_rule *flow_rule) > > > { > > > int i = flow_rule->rule->action.num_entries++; > > > + struct flow_action_entry *entry; > > > + > > > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_ANY != FLOW_ACTION_HW_STATS_ANY); > > > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); > > > + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); > > > + > > > + entry = &flow_rule->rule->action.entries[i]; > > > + entry->hw_stats_type = flow_rule->hw_stats_type; > > Please, use FLOW_ACTION_HW_STATS_ANY by now. Remove the uapi code, > > from this patch. > > > > I'm not sure how users will be using this new knob. > > > > At this stage, I think the drivers knows much better what type to pick > > than the user. > Yes, I agree. > > > > You also have to explain me how you are testing things. > > I test the flowtable offload with mlnx driver. ALL the flow add in driver > failed for checking Thank you for explaining. > the hw_stats_type of flow action. > > > static int parse_tc_fdb_actions(struct mlx5e_priv *priv, > struct flow_action *flow_action, > struct mlx5e_tc_flow *flow, > struct netlink_ext_ack *extack) > { > ................ > > > if (!flow_action_hw_stats_check(flow_action, extack, > FLOW_ACTION_HW_STATS_DELAYED_BIT)) > return -EOPNOTSUPP; > > Indeed always set the hw_stats_type of flow_action to > FLOW_ACTION_HW_STATS_ANY can fix this. Please, do so and do not expose a knob for this yet. > But maybe it can provide a knob for user? Curently with this patch > the user don't need set any value with default value > FLOW_ACTION_HW_STATS in the kernel. My understanding is that hardware has no way to expose how many delayed/immediate counters are available. Users will have to pick based on something it does not know. Many knobs in the kernel are just exposed because developers really do not know how to autoselect the best tuning for their subsystem and they assume users will know better.
diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h index f523ea8..39e1d7e 100644 --- a/include/net/netfilter/nf_flow_table.h +++ b/include/net/netfilter/nf_flow_table.h @@ -43,6 +43,7 @@ struct nf_flow_match { struct nf_flow_rule { struct nf_flow_match match; struct flow_rule *rule; + u8 hw_stats_type; }; struct nf_flowtable_type { @@ -72,6 +73,7 @@ struct nf_flowtable { const struct nf_flowtable_type *type; struct delayed_work gc_work; unsigned int flags; + u8 hw_stats_type; struct flow_block flow_block; struct mutex flow_block_lock; /* Guards flow_block */ possible_net_t net; diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h index 9c3d2d0..9e08c42 100644 --- a/include/uapi/linux/netfilter/nf_tables.h +++ b/include/uapi/linux/netfilter/nf_tables.h @@ -10,6 +10,12 @@ #define NFT_USERDATA_MAXLEN 256 #define NFT_OSF_MAXGENRELEN 16 +#define NFTA_HW_STATS_TYPE_IMMEDIATE (1 << 0) +#define NFTA_HW_STATS_TYPE_DELAYED (1 << 1) + +#define NFTA_HW_STATS_TYPE_ANY (NFTA_HW_STATS_TYPE_IMMEDIATE | \ + NFTA_HW_STATS_TYPE_DELAYED) + /** * enum nft_registers - nf_tables registers * @@ -1560,6 +1566,7 @@ enum nft_object_attributes { * @NFTA_FLOWTABLE_USE: number of references to this flow table (NLA_U32) * @NFTA_FLOWTABLE_HANDLE: object handle (NLA_U64) * @NFTA_FLOWTABLE_FLAGS: flags (NLA_U32) + * @NFTA_FLOWTABLE_HW_STATS_TYPE: hw_stats_type (NLA_BITFIELD32) */ enum nft_flowtable_attributes { NFTA_FLOWTABLE_UNSPEC, @@ -1570,6 +1577,7 @@ enum nft_flowtable_attributes { NFTA_FLOWTABLE_HANDLE, NFTA_FLOWTABLE_PAD, NFTA_FLOWTABLE_FLAGS, + NFTA_FLOWTABLE_HW_STATS_TYPE, __NFTA_FLOWTABLE_MAX }; #define NFTA_FLOWTABLE_MAX (__NFTA_FLOWTABLE_MAX - 1) diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c index ad54931..60289a6 100644 --- a/net/netfilter/nf_flow_table_offload.c +++ b/net/netfilter/nf_flow_table_offload.c @@ -165,8 +165,16 @@ static void flow_offload_mangle(struct flow_action_entry *entry, flow_action_entry_next(struct nf_flow_rule *flow_rule) { int i = flow_rule->rule->action.num_entries++; + struct flow_action_entry *entry; + + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_ANY != FLOW_ACTION_HW_STATS_ANY); + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_IMMEDIATE != FLOW_ACTION_HW_STATS_IMMEDIATE); + BUILD_BUG_ON(NFTA_HW_STATS_TYPE_DELAYED != FLOW_ACTION_HW_STATS_DELAYED); + + entry = &flow_rule->rule->action.entries[i]; + entry->hw_stats_type = flow_rule->hw_stats_type; - return &flow_rule->rule->action.entries[i]; + return entry; } static int flow_offload_eth_src(struct net *net, @@ -602,6 +610,7 @@ int nf_flow_rule_route_ipv6(struct net *net, const struct flow_offload *flow, goto err_flow_match; flow_rule->rule->action.num_entries = 0; + flow_rule->hw_stats_type = flowtable->hw_stats_type; if (flowtable->type->action(net, flow, dir, flow_rule) < 0) goto err_flow_match; diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index f92fb60..27bd6df 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -30,6 +30,7 @@ static LIST_HEAD(nf_tables_destroy_list); static DEFINE_SPINLOCK(nf_tables_destroy_list_lock); static u64 table_handle; +static const u32 nfta_hw_stats_type_allowed = NFTA_HW_STATS_TYPE_ANY; enum { NFT_VALIDATE_SKIP = 0, @@ -6047,6 +6048,9 @@ void nft_unregister_flowtable_type(struct nf_flowtable_type *type) [NFTA_FLOWTABLE_HOOK] = { .type = NLA_NESTED }, [NFTA_FLOWTABLE_HANDLE] = { .type = NLA_U64 }, [NFTA_FLOWTABLE_FLAGS] = { .type = NLA_U32 }, + [NFTA_FLOWTABLE_HW_STATS_TYPE] = { .type = NLA_BITFIELD32, + .validation_data = + &nfta_hw_stats_type_allowed }, }; struct nft_flowtable *nft_flowtable_lookup(const struct nft_table *table, @@ -6244,6 +6248,15 @@ static int nft_register_flowtable_net_hooks(struct net *net, return err; } +static u8 nft_hw_stats_type(const struct nlattr *hw_stats_type_attr) +{ + struct nla_bitfield32 hw_stats_type_bf; + + hw_stats_type_bf = nla_get_bitfield32(hw_stats_type_attr); + + return hw_stats_type_bf.value; +} + static int nf_tables_newflowtable(struct net *net, struct sock *nlsk, struct sk_buff *skb, const struct nlmsghdr *nlh, @@ -6251,6 +6264,7 @@ static int nf_tables_newflowtable(struct net *net, struct sock *nlsk, struct netlink_ext_ack *extack) { const struct nfgenmsg *nfmsg = nlmsg_data(nlh); + u8 hw_stats_type = NFTA_HW_STATS_TYPE_ANY; const struct nf_flowtable_type *type; u8 genmask = nft_genmask_next(net); int family = nfmsg->nfgen_family; @@ -6318,6 +6332,12 @@ static int nf_tables_newflowtable(struct net *net, struct sock *nlsk, goto err3; } + if ((flowtable->data.flags & NF_FLOWTABLE_HW_OFFLOAD) && + nla[NFTA_FLOWTABLE_HW_STATS_TYPE]) + hw_stats_type = + nft_hw_stats_type(nla[NFTA_FLOWTABLE_HW_STATS_TYPE]); + flowtable->data.hw_stats_type = hw_stats_type; + write_pnet(&flowtable->data.net, net); flowtable->data.type = type; err = type->init(&flowtable->data); @@ -6439,6 +6459,17 @@ static int nf_tables_fill_flowtable_info(struct sk_buff *skb, struct net *net, nla_put_be32(skb, NFTA_FLOWTABLE_FLAGS, htonl(flowtable->data.flags))) goto nla_put_failure; + if (flowtable->data.hw_stats_type != NFTA_HW_STATS_TYPE_ANY) { + struct nla_bitfield32 hw_stats_type = { + flowtable->data.hw_stats_type, + NFTA_HW_STATS_TYPE_ANY, + }; + + if (nla_put(skb, NFTA_FLOWTABLE_HW_STATS_TYPE, + sizeof(hw_stats_type), &hw_stats_type)) + goto nla_put_failure; + } + nest = nla_nest_start_noflag(skb, NFTA_FLOWTABLE_HOOK); if (!nest) goto nla_put_failure;