Message-ID: <ZVZV1KLhmuC/Nb2V@t14s.localdomain>
Date: Thu, 16 Nov 2023 14:48:04 -0300
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Jamal Hadi Salim <jhs@...atatu.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

On Thu, Nov 16, 2023 at 10:29:39AM -0500, Jamal Hadi Salim wrote:
> Hi Marcelo,

heya!

> 
> 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?

It is #a.

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

And the part I am missing is, which code are you referring to? :D

This check is being done at tcf_ct_offload_act_setup() because that's
the callback that gets executed when it tries to
serialize/export/whatever act_ct info into flow_action_entry. With
this, it notices that for this instance it can't serialize and aborts
it. AFAIK there isn't an earlier moment on which this check can be
done.

Cheers,
Marcelo

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