Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions

[Vlad Buslov <vladbu@nvidia.com>]

On Sat 30 Oct 2021 at 13:54, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2021-10-29 14:01, Vlad Buslov wrote:
>> On Thu 28 Oct 2021 at 14:06, Simon Horman <simon.horman@corigine.com> wrote:
>>> From: Baowen Zheng <baowen.zheng@corigine.com>
>>>
>>> Add process to validate flags of filter and actions when adding
>>> a tc filter.
>>>
>>> We need to prevent adding filter with flags conflicts with its actions.
>>>
>>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
>>> Signed-off-by: Louis Peens <louis.peens@corigine.com>
>>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>>> ---
>>>   net/sched/cls_api.c      | 26 ++++++++++++++++++++++++++
>>>   net/sched/cls_flower.c   |  3 ++-
>>>   net/sched/cls_matchall.c |  4 ++--
>>>   net/sched/cls_u32.c      |  7 ++++---
>>>   4 files changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>> index 351d93988b8b..80647da9713a 100644
>>> --- a/net/sched/cls_api.c
>>> +++ b/net/sched/cls_api.c
>>> @@ -3025,6 +3025,29 @@ void tcf_exts_destroy(struct tcf_exts *exts)
>>>   }
>>>   EXPORT_SYMBOL(tcf_exts_destroy);
>>>   +static bool tcf_exts_validate_actions(const struct tcf_exts *exts, u32
>>> flags)
>>> +{
>>> +#ifdef CONFIG_NET_CLS_ACT
>>> +	bool skip_sw = tc_skip_sw(flags);
>>> +	bool skip_hw = tc_skip_hw(flags);
>>> +	int i;
>>> +
>>> +	if (!(skip_sw | skip_hw))
>>> +		return true;
>>> +
>>> +	for (i = 0; i < exts->nr_actions; i++) {
>>> +		struct tc_action *a = exts->actions[i];
>>> +
>>> +		if ((skip_sw && tc_act_skip_hw(a->tcfa_flags)) ||
>>> +		    (skip_hw && tc_act_skip_sw(a->tcfa_flags)))
>>> +			return false;
>>> +	}
>>> +	return true;
>>> +#else
>>> +	return true;
>>> +#endif
>>> +}
>>> +
>> I know Jamal suggested to have skip_sw for actions, but it complicates
>> the code and I'm still not entirely understand why it is necessary.
>
> If the hardware can independently accept an action offload then
> skip_sw per action makes total sense. BTW, my understanding is

Example configuration that seems bizarre to me is when offloaded shared
action has skip_sw flag set but filter doesn't. Then behavior of
classifier that points to such action diverges between hardware and
software (different lists of actions are applied). We always try to make
offloaded TC data path behave exactly the same as software and, even
though here it would be explicit and deliberate, I don't see any
practical use-case for this.

> _your_ hardware is capable as such at least for policers ;->
> And such policers are then shared across filters.

True, but why do you need skip_sw action flag for that?

> Other than the architectural reason I may have missed something
> because I dont see much complexity added as a result.

Well, other part of my email was about how I don't understand what is
going on in the flags handling code here. This patch and parts of other
patches in the series would be unnecessary, if we forgo the action
skip_sw flag. I guess we can just make the code nicer by folding the
validation into tcf_action_init(), for example.

> Are you more worried about slowing down the update rate?

I don't expect the validation code to significantly impact the update
rate.