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!
--
davide
