Re: [PATCH nf-next] netfilter:nf_flow_table: add HW stats type support in flowtable

[wenxu <wenxu@ucloud.cn>]

在 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.
>