> -----Original Message-----
> From: Eelco Chaudron <[email protected]>
> Sent: Thursday 7 July 2022 14:54
> To: Finn, Emma <[email protected]>
> Cc: Ilya Maximets <[email protected]>; Van Haaren, Harry
> <[email protected]>; [email protected]; Stokes, Ian
> <[email protected]>; Flavio Leitner <[email protected]>; Mcnamara, John
> <[email protected]>
> Subject: Re: [ovs-dev] [PATCH v7 02/11] odp-execute: Add function pointers
> to odp-execute for different action implementations.
>
>
>
> On 7 Jul 2022, at 15:31, Finn, Emma wrote:
>
> <SNIP>
>
> >
> > Hi Eelco/Ilya,
> >
> > Thanks for the reviews.
> >
> > I have reworked this series based on all your feedback and I have a V8
> > ready to push.
> >
> > Checksum handling has been reworked to update checksum and not
> > recalculate And now follows the same method as scalar checksum update.
> > The first patch in this series has been removed and all unit tests are
> passing.
> > The dependency on the userspace datapath has also been removed and
> > feature has been marked as experimental. Plus all other smaller review
> > comments have been taken on board and no open discussions on the v7
> are left.
> >
> > If you are okay with this, I will send v8 to the mailing list.
>
> Hi Emma,
>
> If all the items I brought up during the review are taken care of, and none of
> them require further discussion, please go ahead, and send out the v8.
>
> If some things were not addressed or are implemented differently than
> suggested, let’s first discuss them.
>
> Cheers,
>
> Eelco
There is only one comment where your suggestion has been implemented slightly
differently.
As there was an issue with mails bouncing I cannot reply inline as I have been
working off
patchwork. But I will snip the info in here.
> > diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> > index f8d0896b5..de2e4dfc4 100644
> > --- a/lib/odp-execute-private.c
> > +++ b/lib/odp-execute-private.c
> > @@ -42,6 +42,14 @@ static struct odp_execute_action_impl action_impls[] = {
> > .name = "scalar",
> > .init_func = odp_action_scalar_init,
> > },
> > +
> > + #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
>
> From the v6 discussion:
>
> EC> How about changing this instance to #ifdef ACTION_IMPL_AVX512? This way
> we only have one place where we have these compiler/arch checks.
>
> EF> V7 will not include a fix here, but we will investigate and report back
> to OVS ML with results of investigation.
>
> Not sure why you need further investigation? If ACTION_IMPL_AVX512 was not
> defined the above flags where false already in odp-execute-private.h.
> I think the change should simply be this:
>
> - #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
> + #ifdef ACTION_IMPL_AVX512
>
> Or am I missing something? I see one version on gcc complain about this, is
> this what you are figuring out?
>
> gcc (GCC) 11.3.1 20220421 works fine, gcc (GCC) 11.2.1 20220127 seems to
> report an issues:
>
> lib/odp-execute-private.c:86:9: warning: iteration 2 invokes undefined
> behavior [-Waggressive-loop-optimizations]
>
> I do not see this problem with clang. Also the github actions script compile
> just fine. You might just be as unlucky as I was, and you have a broken
> compile version?
>
> > + [ACTION_IMPL_AVX512] = {
> > + .available = false,
> > + .name = "avx512",
> > + .init_func = action_avx512_init,
> > + },
> > + #endif
> > };
> >
The #ifdef ACTION_IMPL_AVX512 is checking an enum in the C code, I didn’t think
this was possibly and I tried but couldn't get this to work.
As an alternative I refactored the compile checks to a similar way done in this
patch from Sunil -
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
This will simplify the build time checks around the code.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
