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.
