Re: [PATCH net-next v3 1/2] net: flow_offload: add tc police action parameters

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Mar 17, 2022 at 03:22:42PM +0200, Ido Schimmel wrote:
> > I don't know why just now, but I observed an apparent regression here
> > with these commands:
> > 
> > root@debian:~# tc qdisc add dev swp3 clsact
> > root@debian:~# tc filter add dev swp3 ingress protocol ip flower skip_sw ip_proto icmp action police rate 100Mbit burst 10000
> > [   45.767900] tcf_police_act_to_flow_act: 434: tc_act 1
> > [   45.773100] tcf_police_offload_act_setup: 475, act_id -95
> > Error: cls_flower: Failed to setup flow action.
> > We have an error talking to the kernel, -1
> > 
> > The reason why I'm not sure is because I don't know if this should have
> > worked as intended or not. I am remarking just now in "man tc-police"
> > that the default conform-exceed action is "reclassify".
> > 
> > So if I specify "conform-exceed drop", things are as expected, but with
> > the default (implicitly "conform-exceed reclassify") things fail with
> > -EOPNOTSUPP because tcf_police_act_to_flow_act() doesn't handle a
> > police->tcf_action of TC_ACT_RECLASSIFY.
> > 
> > Should it?
> 
> Even if tcf_police_act_to_flow_act() handled "reclassify", the
> configuration would have been rejected later on by the relevant device
> driver since they all support "drop" for exceed action and nothing else.

This is correct, but currently, the error is:

Error: cls_flower: Failed to setup flow action.
We have an error talking to the kernel, -1

I'd appreciate if the error was instead:

Error: mscc_ocelot: Offload not supported when exceed action is not drop.

which is basically what Jianbo was trying to achieve when he added the
policer_validate() functions. At least I'd know what's wrong. No?

> I don't know why iproute2 defaults to "reclassify", but the
> configuration in the example does something different in the SW and HW
> data paths. One ugly suggestion to keep this case working it to have
> tcf_police_act_to_flow_act() default to "drop" and emit a warning via
> extack so that user space is at least aware of this misconfiguration.

I don't want to force a reinterpretation of "reclassify" just to make
something that used to work by mistake continue to work. It sucks to
have to adapt, but not being able to make progress because of such
things sucks even more.

I'd just like the 'reclassify' action to be propagated in some reasonable
way to flow offload, considering that at the moment the error is quite cryptic.

> > > +		if (act_id < 0)
> > > +			return act_id;
> > > +
> > > +		entry->police.exceed.act_id = act_id;
> > > +
> > > +		act_id = tcf_police_act_to_flow_act(p->tcfp_result,
> > > +						    &entry->police.notexceed.extval);
> > > +		if (act_id < 0)
> > > +			return act_id;
> > > +
> > > +		entry->police.notexceed.act_id = act_id;
> > > +
> > >  		*index_inc = 1;
> > >  	} else {
> > >  		struct flow_offload_action *fl_action = entry_data;
> > > -- 
> > > 2.26.2
> > >