Date:   Fri, 2 Dec 2022 12:33:10 +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 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.
> 
> - 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.

Thanks,
Tianyu
> 
> //Eelco
> 
> > 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