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 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
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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
