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@xxxxxxxxx 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.
