> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Friday 1 July 2022 13:35 > To: Eelco Chaudron <[email protected]>; Van Haaren, Harry > <[email protected]>; Finn, Emma <[email protected]> > Cc: [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 6/30/22 14:35, Eelco Chaudron wrote: > > > > > > On 30 Jun 2022, at 14:13, Van Haaren, Harry wrote: > > > >>> -----Original Message----- > >>> From: Eelco Chaudron <[email protected]> > >>> Sent: Thursday, June 30, 2022 12:44 PM > >>> To: Van Haaren, Harry <[email protected]> > >>> Cc: Finn, Emma <[email protected]>; Stokes, Ian > >>> <[email protected]>; [email protected] > >>> Subject: Re: [PATCH v7 02/11] odp-execute: Add function pointers to > >>> odp-execute for different action implementations. > >>> > >>> > >>> > >>> On 30 Jun 2022, at 10:16, Van Haaren, Harry wrote: > >>> > >>>>> -----Original Message----- > >>>>> From: Eelco Chaudron <[email protected]> > >>>>> Sent: Thursday, June 23, 2022 4:38 PM > >>>>> To: Finn, Emma <[email protected]> > >>>>> Cc: Stokes, Ian <[email protected]>; Van Haaren, Harry > >>>>> <[email protected]>; [email protected] > >>>>> Subject: Re: [PATCH v7 02/11] odp-execute: Add function pointers > >>>>> to odp-execute for different action implementations. > >>>>> > >>>>> On 14 Jun 2022, at 13:57, Emma Finn wrote: > >>>> > >>>> <snip patch contents> > >>>> > >>>>>> -- > >>>>>> 2.32.0 > >>>>> > >>>>> > >>>>> General discussion item from v6: > >>>> > >>>> Thanks Eelco for the detailed review. > >>>> > >>>> > >>>>> EC> I have another general architecture question? What if for a > >>>>> EC> specific packet > >>> (or > >>>>> batch of packets) the SIMD implementation is not able to process > >>>>> the > >>> packet/batch? > >>>>> EC> How can we handle this in a uniform way? Can you give this > >>>>> EC> some thought > >>> and > >>>>> add support? > >>>>> > >>>>> EF> We support 100% of the action (regardless of input packet, we > >>>>> EF> perform same > >>>>> action as scalar actions code). Autovalidation ensures the > >>>>> modifications are identical. > >>>>> EF> As a result there is no value in a "fallback to scalar" > >>>>> EF> backup, as this must never > >>>>> occur. > >>>>> > >>>>> What I'm trying to say is, that in certain cases, your SIMD > >>>>> specific code might not > >>> be > >>>>> able to optimize the action handling, and it might want to fall > >>>>> back to the scaler implementation. > >>>> > >>>> Today, the AVX512 implementation in this patchset will *always* be > >>>> able to handle > >>> the action. > >>>> There is no requirement for scalar fallback. > >>> > >>> Well actually there is a need, but don’t worry I fixed/added it to my > patch 10 review. > >>> > >>> Take a look at the quick fix here, > >>> https://mail.openvswitch.org/pipermail/ovs- > >>> dev/2022-June/395068.html, where some scalar functions get called. > >> > >> Specifically, you mean this bit? > >> > >> @@ -411,6 +412,13 @@ action_avx512_set_masked(struct > dp_packet_batch > >> *batch OVS_UNUSED, > >> > >> if (avx512_impl.set_masked_funcs[attr_type]) { > >> avx512_impl.set_masked_funcs[attr_type](batch, a); > >> + } else { > >> + struct dp_packet *packet; > >> + a = nl_attr_get(a); > >> + > >> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > >> + odp_execute_masked_set_action(packet, a); > >> + } > >> } > >> > >> I don't see that as a *fallback* because something couldn't be > >> handled, but a "use-scalar" because the AVX512 isn't implemented (func > ptr = NULL). > >> > >> It's not the case that the AVX512 couldn’t handle a specific input > >> packet here; the case is that Action has SubActions, and all are not > implemented yet. > >> > >> Most Actions do NOT have SubActions (like SET_MASKED has). I think > >> it's OK to handle the "use-scalar" in these few cases. > >> > >> My main concern was around requirements hypothetical OpenCL > implementations. > >> To handle in a generic way at the Action level (instead of SubAction > >> like above) seems to over-complicate things to me... > >> > >> Input would be useful to quickly find path forward and get that built & > merged. > >> Is the above OK for 2.18, and refactor/rework if required for > 2.19/onwards? > > > > Hi Harry/Emma, > > > > No problem, as an exception to speed things up for this patch set, I’ve > created patches on top of Emma's patches with (almost) all the changes I > would deem needed for approval. Guess the exception is the checksum > handling in patch 11. And some potential AVX optimisations. > > > > If anything in these changes I proposed is not acceptable, let's discuss > > this > before you sent out a new revision. Same for the open items I did not fix, > please either ACK or NACK the suggested changes. If NACKed, let's discuss > them first before sending the next revision, so we can hopefully have one > final rev. > > > > > > //Eelco > > Amber reminded me about artificial dependencies in AVX512 code, so I also > took a look at this patch set again. > > Just want to highlight that one of the main comments from v4 that I asked > you to address back in January wasn't actually addressed. > > I read only a couple of relevant patches, but I see that the optimization > still > depends on userspace datapath in v7. And it is a purely artificial > restriction, > because all the actual code is placed outside of dpif-netdev and has no > dependencies on the datapath type, AFAICT. Also if the change is enabled in > the dpif-netdev, it will also affect all other datapaths serviced by the same > daemon. So, it can not be enabled for other datapaths, but affects them if > enabled. > > Please, move all the code from dpif-netdev modules somewhere else and > re-name unixctl commands, so they don't have dpif-netdev in their names. I > think, it's a critical thing to fix, as it will be harder to do once the > change is > released. Luckily, it seems to be just a matter of moving and re-naming a > couple of functions. > > It also, probably, makes sense to clearly mark these features as experimental > in the docs, so changes to the API can be made more easily in the future, if > required. > > Best regards, Ilya Maximets.
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. Thanks, Emma _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
