* [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable
@ 2020-03-20 7:34 wenxu
2020-03-20 12:10 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: wenxu @ 2020-03-20 7:34 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel
From: wenxu <wenxu@ucloud.cn>
The hardware driver offload function will check the hw_stats_type
for the flow action. Add NFTA_FLOWTABLE_HW_STATS_TYPE attr to flowtable
to specify the type of HW stats.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
include/net/netfilter/nf_flow_table.h | 2 ++
include/uapi/linux/netfilter/nf_tables.h | 8 ++++++++
net/netfilter/nf_flow_table_offload.c | 11 ++++++++++-
net/netfilter/nf_tables_api.c | 31 +++++++++++++++++++++++++++++++
4 files changed, 51 insertions(+), 1 deletion(-)
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;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable
2020-03-20 7:34 [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable wenxu
@ 2020-03-20 12:10 ` Pablo Neira Ayuso
2020-03-20 12:36 ` wenxu
0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2020-03-20 12:10 UTC (permalink / raw)
To: wenxu; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable
2020-03-20 12:10 ` Pablo Neira Ayuso
@ 2020-03-20 12:36 ` wenxu
2020-03-20 13:03 ` Pablo Neira Ayuso
0 siblings, 1 reply; 4+ messages in thread
From: wenxu @ 2020-03-20 12:36 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
在 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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable
2020-03-20 12:36 ` wenxu
@ 2020-03-20 13:03 ` Pablo Neira Ayuso
0 siblings, 0 replies; 4+ messages in thread
From: Pablo Neira Ayuso @ 2020-03-20 13:03 UTC (permalink / raw)
To: wenxu; +Cc: netfilter-devel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-20 13:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 7:34 [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable wenxu
2020-03-20 12:10 ` Pablo Neira Ayuso
2020-03-20 12:36 ` wenxu
2020-03-20 13:03 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).