> -----Original Message-----
> From: Stokes, Ian <ian.sto...@intel.com>
> Sent: Wednesday, January 12, 2022 9:47 PM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; ovs-
> d...@openvswitch.org
> Cc: i.maxim...@ovn.org; Amber, Kumar <kumar.am...@intel.com>
> Subject: RE: [PATCH v5 2/2] dpif-netdev/mfex: Optimize packet hash and enable
> autovalidator
>
> > From: Kumar Amber <kumar.am...@intel.com>
> >
> > This patch adds error checking of packet hashes to the mfex
> > autovalidator infrastructure, ensuring that hashes calculated by
> > optimized mfex implementations is identical to the scalar code.
> >
> > This patch avoids calculating the software hash of the packet again
> > if the optimized miniflow-extract hit and has already calculated the
> > packet hash. In cases of scalar miniflow extract, the normal hashing
> > calculation is performed.
> >
> > Signed-off-by: Kumar Amber <kumar.am...@intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
>
> Thanks for the patch Harry/Amber, few queries below.
Thanks for review Ian.
<snip>
> > diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> > index b7131ba3f..c68b79f6b 100644
> > --- a/lib/dpif-netdev-avx512.c
> > +++ b/lib/dpif-netdev-avx512.c
> > @@ -212,15 +212,15 @@ dp_netdev_input_outer_avx512(struct
> > dp_netdev_pmd_thread *pmd,
> > if (!mfex_hit) {
> > /* Do a scalar miniflow extract into keys. */
> > miniflow_extract(packet, &key->mf);
> > + key->len = netdev_flow_key_size(miniflow_n_values(&key->mf));
> > + key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
> > + &key->mf);
>
> So I'm not sure, but has there been any investigation into the effect of only
> storing this info when !mfex_hit occurs?
Yes, good question. The Optimized/SIMD MFEX implementation sets the
key->len and key->hash when we have a mfex hit.
Hence, setting these values using the scalar approach as above is only required
when the optimized mfex "misses".
> Prior to this these values were stored regardless. My concern here is that is
> there
> a case where this info is needed even if the mfex_hit is true?
Correct, the information key->len and key->hash is "always" required.
(Always in quotes, as e.g. EMC needs the hash, however EMC could be disabled.)
<snip>
> > diff --git a/lib/dpif-netdev-private-extract.c
> > b/lib/dpif-netdev-private-extract.c
> > index a29bdcfa7..2957c0172 100644
> > --- a/lib/dpif-netdev-private-extract.c
> > +++ b/lib/dpif-netdev-private-extract.c
> > @@ -252,8 +252,15 @@ dpif_miniflow_extract_autovalidator(struct
> > dp_packet_batch *packets,
> >
> > /* Run scalar miniflow_extract to get default result. */
> > DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> > +
> > + /* remove the NIC RSS bit to force SW hashing for validation. */
> Minor, Capitalize Remove.
> > + dp_packet_reset_offload(packet);
> > +
> Is there any performance penalty for forcing this reset each time?
Its an inline function in dp-packet.h, which results in a bitwise operation,
so "no" is the simple answer.
Other thing to note is that this is the autovalidator for testing, and will
not be the common path in production environments, so performance
is not critical here.
> Ian
Regards, -Harry
<snip>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
