Message-ID: <PH0PR13MB4793B28A3E695512BD469DCA94D29@PH0PR13MB4793.namprd13.prod.outlook.com>
Date:   Sun, 29 Jan 2023 08:16:56 +0000
From:   Tianyu Yuan <tianyu.yuan@...igine.com>
To:     Eelco Chaudron <echaudro@...hat.com>
CC:     Marcelo Leitner <mleitner@...hat.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Davide Caratti <dcaratti@...hat.com>,
        Edward Cree <edward.cree@....com>,
        Ilya Maximets <i.maximets@....org>,
        Oz Shlomo <ozsh@...dia.com>, Paul Blakey <paulb@...dia.com>,
        Vlad Buslov <vladbu@...dia.com>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        oss-drivers <oss-drivers@...igine.com>,
        Ziyang Chen <ziyang.chen@...igine.com>
Subject: RE: [PATCH/RFC net-next] tc: allow drivers to accept gact with PIPE
 when offloading

On 2 Dec 2022, at 8:40 PM, Eelco Chaudron wrote:
> 
> On 2 Dec 2022, at 13:33, Tianyu Yuan wrote:
> 
> > On Fri, Dec 2, 2022 at 8:18 PM , Eelco Chaudron wrote:
> >>
> >> On 30 Nov 2022, at 4:36, Tianyu Yuan wrote:
> >>
> >>> On Mon, Nov 29, 2022 at 8:35 PM , Eelco Chaudron wrote:
> >>>>
> >>>> On 28 Nov 2022, at 14:33, Marcelo Leitner wrote:
> >>>>
> >>>>> On Mon, Nov 28, 2022 at 02:17:40PM +0100, Eelco Chaudron wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 28 Nov 2022, at 14:11, Marcelo Leitner wrote:
> >>>>>>
> >>>>>>> On Mon, Nov 28, 2022 at 07:11:05AM +0000, Tianyu Yuan wrote:
> >>>>> ...
> >>>>>>>>
> >>>>>>>> Furthermore, I think the current stats for each action
> >>>>>>>> mentioned in
> >>>>>>>> 2) cannot represent the real hw stats and this is why [ RFC
> >>>>>>>> net-next v2 0/2] (net: flow_offload: add support for per action
> >>>>>>>> hw stats)
> >>>> will come up.
> >>>>>>>
> >>>>>>> Exactly. Then, when this patchset (or similar) come up, it won't
> >>>>>>> update all actions with the same stats anymore. It will require
> >>>>>>> a set of stats from hw for the gact with PIPE action here. But
> >>>>>>> if drivers are ignoring this action, they can't have specific
> >>>>>>> stats for it. Or am I missing something?
> >>>>>>>
> >>>>>>> So it is better for the drivers to reject the whole flow instead
> >>>>>>> of simply ignoring it, and let vswitchd probe if it should or
> >>>>>>> should not use this action.
> >>>>>>
> >>>>>> Please note that OVS does not probe features per interface, but
> >>>>>> does it
> >>>> per datapath. So if it’s supported in pipe in tc software, we will
> >>>> use it. If the driver rejects it, we will probably end up with the
> >>>> tc software
> >> rule only.
> >>>>>
> >>>>> Ah right. I remember it will pick 1 interface for testing and use
> >>>>> those results everywhere, which then I don't know if it may or may
> >>>>> not be a representor port or not. Anyhow, then it should use
> >>>>> skip_sw, to try to probe for the offloading part. Otherwise I'm
> >>>>> afraid tc sw will always accept this flow and trick the probing, yes.
> >>>>
> >>>> Well, it depends on how you look at it. In theory, we should be
> >>>> hardware agnostic, meaning what if you have different hardware in
> >>>> your system? OVS only supports global offload enablement.
> >>>>
> >>>> Tianyu how are you planning to support this from the OVS side? How
> >>>> would you probe kernel and/or hardware support for this change?
> >>>
> >>> Currently in the test demo, I just extend gact with PIPE (previously
> >>> only SHOT as default and GOTO_CHAIN when chain exists), and then put
> >>> such a gact with PIPE at the first place of each filter which will
> >>> be transacted
> >> with kernel tc.
> >>>
> >>> About the tc sw datapath mentioned, we don't have to make changes
> >>> because gact with PIPE has already been supported in current tc
> >>> implementation and it could act like a 'counter' And for the
> >>> hardware we just need to ignore this PIPE and the stats of this
> >>> action will still be
> >> updated in kernel side and sent to userspace.
> >>
> >> I know it’s supported now, but if we implement it, it might fail in
> >> existing environments. So from an OVS userspace perspective, you need
> >> to implement something like:
> >
> > I've got your point now, sorry for my misunderstanding previously.
> 
> No problem, there are quite some emails around this patch.
> 
> >> - Probe the kernel to see if this patch is applied, if not use the
> >> old method so we do not break existing deployments when upgrading
> OVS
> >> but not the kernel.
> >> - If we do have this newer kernel, do we assume all drivers that
> >> worked before, now also work?
> >>   - If this is not the case, how will you determine what approach to
> >> use? We do not have a per-interface layer, but a per-datapath one,
> >> i.e. the kernel. We do not know at initialization time what NICs will
> >> be added later and we can not decide on the strategy to use.
> >>
> >> Thought? Maybe this should be discussed outside of the netdev mailing
> >> list, but what I want to highlight is that there should be a runtime
> >> way to determine if this patch is applied to the kernel (without
> >> using any actual hw driver).
> >
> > I agree that whether the patch is applied in kernel should be checked
> > at runtime rather than compiling (for the demo I made this check
> > inacinlude.m4). I think I need some time to investigate how to implement it.
> We may discuss it later in an OVS mailing list.
> 
> No problem, but just want to make sure that if it needs changes to this patch
> to be able to do it, now is the time ;)

Sorry to not reply for such a long time.

I made some investigation on how to probe single pipe action offload support in
OVS side and found that we don't have to make change in kernel for this probe
purpose.

Maybe we could process this kernel tc and driver changes after OVS one.
Look forward to discussing with you in OVS side 😊

Cheers,
Tianyu

> 
> 
> >>> I agree with that the unsupported actions should be rejected by
> >>> drivers, so may another approach could work without ignoring PIPE in
> >>> all the related drivers, that we directly make put the flower stats
> >>> from driver into the socket which is used to transact with userspace
> >>> and
> >> userspace(e.g. OVS) update the flow stats using this stats instead of
> >> the parsing the action stats. How do you think of this?

Powered by blists - more mailing lists