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 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 some thought >> and >>>> add support? >>>> >>>> EF> We support 100% of the action (regardless of input packet, we 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" 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
