Date: Thu, 16 Nov 2023 10:29:39 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc: Xin Long <lucien.xin@...il.com>, network dev <netdev@...r.kernel.org>, davem@...emloft.net, 
	kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, 
	Vladyslav Tarasiuk <vladyslavt@...dia.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net] net: sched: do not offload flows with a helper in act_ct

Hi Marcelo,

On Thu, Nov 16, 2023 at 6:36 AM Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
>
> On Wed, Nov 15, 2023 at 11:37:51AM -0500, Jamal Hadi Salim wrote:
> > Hi Xin,
> >
> > On Tue, Nov 14, 2023 at 10:18 AM Xin Long <lucien.xin@...il.com> wrote:
> > >
> > > On Mon, Nov 13, 2023 at 4:37 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > > >
> > > > On Mon, Nov 13, 2023 at 12:53 PM Xin Long <lucien.xin@...il.com> wrote:
> > > > >
> > > > > There is no hardware supporting ct helper offload. However, prior to this
> > > > > patch, a flower filter with a helper in the ct action can be successfully
> > > > > set into the HW, for example (eth1 is a bnxt NIC):
> > > > >
> > > > >   # tc qdisc add dev eth1 ingress_block 22 ingress
> > > > >   # tc filter add block 22 proto ip flower skip_sw ip_proto tcp \
> > > > >     dst_port 21 ct_state -trk action ct helper ipv4-tcp-ftp
> > > > >   # tc filter show dev eth1 ingress
> > > > >
> > > > >     filter block 22 protocol ip pref 49152 flower chain 0 handle 0x1
> > > > >       eth_type ipv4
> > > > >       ip_proto tcp
> > > > >       dst_port 21
> > > > >       ct_state -trk
> > > > >       skip_sw
> > > > >       in_hw in_hw_count 1   <----
> > > > >         action order 1: ct zone 0 helper ipv4-tcp-ftp pipe
> > > > >          index 2 ref 1 bind 1
> > > > >         used_hw_stats delayed
> > > > >
> > > > > This might cause the flower filter not to work as expected in the HW.
> > > > >
> > > > > This patch avoids this problem by simply returning -EOPNOTSUPP in
> > > > > tcf_ct_offload_act_setup() to not allow to offload flows with a helper
> > > > > in act_ct.
> > > > >
> > > > > Fixes: a21b06e73191 ("net: sched: add helper support in act_ct")
> > > > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > > >
> > > > I didnt quite follow:
> > > > The driver accepted the config, but the driver "kind of '' supports
> > > > it. (enough to not complain and then display it when queried).
> > > > Should the driver have rejected something it doesnt fully support?
> > > Hi, Jamal,
> > >
> > > The sad thing is now it does not pass the 'helper' param to the HW in
> > > tcf_ct_offload_act_setup() via struct flow_action_entry, in fact
> > > flow_action_entry does not even have a member to keep 'helper'.
> > >
> > > Since no HW currently supports 'helper', we can stop it setting to HW
> > > from here for now. In future, if HWs and struct flow_action_entry
> > > support it, we can set it to the entry and reply on HWs to reject
> > > it when not supported, as you mentioned above.
> >
> > That makes sense - so i am wondering why that was ever added there to
> > begin with. Would there be any hardware that would have any helper
> > support? If no, Shouldnt that code be deleted altogether?
>
> Not sure if I follow you, Jamal. There's no code at all to pass the
> helper information down to the drivers. So drivers ended up accepting
> this flow because they had no idea that a helper was attached to it.
>

So is the goal:
a) if there's a helper it doesnt make sense to offload the flow or
b) if there's a helper then it(the helper) works in s/w only but the
flow offload is still legit?

If it is #a then my question was why is that code even there in the
offload path...
Likely i am missing something..

cheers,
jamal

> Then yes, ideally, it should be driver the one to reject the flow that
> it doesn't support. But as currently zero drivers support it, and I
> doubt one will in the future [*], this patch simplifies it by instead
> of adding all the helper stuff to flow_action_entry, to just abort the
> offload earlier.
>
> [*] it requires parsing TCP payload, including over packet boundaries.
> This is very expensive in hw. And leads to another problem: the HW
> having to tell the SW stack about new conntrack expectations.
>


> Chers,
> Marcelo
>
> >
> > In any case, to the code correctness:
> > Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> >
> > cheers,
> > jamal
> > > Thanks.
> > > >
> > > >
> > > > cheers,
> > > > jamal
> > > >
> > > > > ---
> > > > >  include/net/tc_act/tc_ct.h | 9 +++++++++
> > > > >  net/sched/act_ct.c         | 3 +++
> > > > >  2 files changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/include/net/tc_act/tc_ct.h b/include/net/tc_act/tc_ct.h
> > > > > index 8a6dbfb23336..77f87c622a2e 100644
> > > > > --- a/include/net/tc_act/tc_ct.h
> > > > > +++ b/include/net/tc_act/tc_ct.h
> > > > > @@ -58,6 +58,11 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a)
> > > > >         return to_ct_params(a)->nf_ft;
> > > > >  }
> > > > >
> > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a)
> > > > > +{
> > > > > +       return to_ct_params(a)->helper;
> > > > > +}
> > > > > +
> > > > >  #else
> > > > >  static inline uint16_t tcf_ct_zone(const struct tc_action *a) { return 0; }
> > > > >  static inline int tcf_ct_action(const struct tc_action *a) { return 0; }
> > > > > @@ -65,6 +70,10 @@ static inline struct nf_flowtable *tcf_ct_ft(const struct tc_action *a)
> > > > >  {
> > > > >         return NULL;
> > > > >  }
> > > > > +static inline struct nf_conntrack_helper *tcf_ct_helper(const struct tc_action *a)
> > > > > +{
> > > > > +       return NULL;
> > > > > +}
> > > > >  #endif /* CONFIG_NF_CONNTRACK */
> > > > >
> > > > >  #if IS_ENABLED(CONFIG_NET_ACT_CT)
> > > > > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > > > > index 0db0ecf1d110..b3f4a503ee2b 100644
> > > > > --- a/net/sched/act_ct.c
> > > > > +++ b/net/sched/act_ct.c
> > > > > @@ -1549,6 +1549,9 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data,
> > > > >         if (bind) {
> > > > >                 struct flow_action_entry *entry = entry_data;
> > > > >
> > > > > +               if (tcf_ct_helper(act))
> > > > > +                       return -EOPNOTSUPP;
> > > > > +
> > > > >                 entry->id = FLOW_ACTION_CT;
> > > > >                 entry->ct.action = tcf_ct_action(act);
> > > > >                 entry->ct.zone = tcf_ct_zone(act);
> > > > > --
> > > > > 2.39.1
> > > > >

Powered by blists - more mailing lists