Re: [PATCH net-next v3] net: openvswitch: Set OvS recirc_id from tc chain index

[Paul Blakey <paulb@mellanox.com>]

On 9/4/2019 12:47 PM, Davide Caratti wrote:
> On Tue, 2019-09-03 at 16:23 +0300, Paul Blakey wrote:
>> Offloaded OvS datapath rules are translated one to one to tc rules,
>> for example the following simplified OvS rule:
>>
>> recirc_id(0),in_port(dev1),eth_type(0x0800),ct_state(-trk) actions:ct(),recirc(2)
>>
>> Will be translated to the following tc rule:
>>
>> $ tc filter add dev dev1 ingress \
>> 	    prio 1 chain 0 proto ip \
>> 		flower tcp ct_state -trk \
>> 		action ct pipe \
>> 		action goto chain 2
> hello Paul!
>
> one small question:
>
> [... ]
>
>> index 43f5b7e..2fdc746 100644
>> --- a/include/net/sch_generic.h
>> +++ b/include/net/sch_generic.h
>> @@ -274,7 +274,10 @@ struct tcf_result {
>>   			unsigned long	class;
>>   			u32		classid;
>>   		};
>> -		const struct tcf_proto *goto_tp;
>> +		struct {
>> +			const struct tcf_proto *goto_tp;
>> +			u32 goto_index;
> I don't understand why we need to store another copy of the chain index in
> 'res.goto_index'.
> (see below)
>
> [...]
>
>> index 3397122..c393604 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -27,6 +27,7 @@ static void tcf_action_goto_chain_exec(const struct tc_action *a,
>>   {
>>   	const struct tcf_chain *chain = rcu_dereference_bh(a->goto_chain);
>>   
>> +	res->goto_index = chain->index;
> I see "a->goto_chain" is used to read the chain index, but I think it's
> not needed _ because the chain index is encoded together with the "goto
> chain" control action.
>
>>   	res->goto_tp = rcu_dereference_bh(chain->filter_chain);
>>   }
>>   
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 671ca90..dd147be 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -1514,6 +1514,18 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>   			goto reset;
>>   		} else if (unlikely(TC_ACT_EXT_CMP(err, TC_ACT_GOTO_CHAIN))) {
>>   			first_tp = res->goto_tp;
>> +
>> +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
>> +			{
>> +				struct tc_skb_ext *ext;
>> +
>> +				ext = skb_ext_add(skb, TC_SKB_EXT);
>> +				if (WARN_ON_ONCE(!ext))
>> +					return TC_ACT_SHOT;
>> +
>> +				ext->chain = res->goto_index;
> the value of 'res->goto_index' is already encoded in the control action
> 'err' (masked with TC_ACT_EXT_VAL_MASK), since TC_ACT_GOTO_CHAIN bits are
> not zero.
>
> you can just get rid of res->goto_index, and just do:
>
> 	ext->chain = err & TC_ACT_EXT_VAL_MASK;
>
> am I missing something?
>
> thanks!

No, good catch :) Thanks.

tcf_action_set_ctrlact sets the action with the chain index on tc action 
instance (tcf_action), so yes we can access it just like you say.

I'll send a fix.