On 7 Jul 2022, at 16:20, Finn, Emma wrote:
>> -----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. Guess this enum part might be the reason it was working with some compiler version but not with an older one. I’ll take another look at this in your v8, as I still need to take a look at Sunil’s patch… Looking forward to your v8. //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
