[ovs-dev,v5,2/2] dpif-netdev/mfex: Optimize packet hash and enable autovalidator

From: Kumar Amber <kumar.amber@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.amber@intel.com>
Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

v5:
- Always use SW hashing to validate optimized hash implementations
---
 lib/dpif-netdev-avx512.c          |  6 +++---
 lib/dpif-netdev-private-extract.c | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

Bleep bloop.  Greetings Harry van Haaren, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Harry van Haaren <harry.van.haaren@intel.com>
Lines checked: 95, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

> From: Kumar Amber <kumar.amber@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.amber@intel.com>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Thanks for the patch Harry/Amber, few queries below.

> 
> ---
> 
> v5:
> - Always use SW hashing to validate optimized hash implementations
> ---
>  lib/dpif-netdev-avx512.c          |  6 +++---
>  lib/dpif-netdev-private-extract.c | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> 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?
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?
>          }
> 
>          /* Cache TCP and byte values for all packets. */
>          pkt_meta[i].bytes = dp_packet_size(packet);
>          pkt_meta[i].tcp_flags = miniflow_get_tcp_flags(&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);
> -
>          if (emc_enabled) {
>              f = emc_lookup(&cache->emc_cache, key);
> 
> 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?

Ian
>          pkt_metadata_init(&packet->md, in_port);
>          miniflow_extract(packet, &keys[i].mf);
> +        keys[i].len = netdev_flow_key_size(miniflow_n_values(&keys[i].mf));
> +        keys[i].hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet,
> +                                                                &keys[i].mf);
> 
>          /* Store known good metadata to compare with optimized metadata. */
>          good_l2_5_ofs[i] = packet->l2_5_ofs;
> @@ -271,7 +278,10 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
>          /* Reset keys and offsets before each implementation. */
>          memset(test_keys, 0, keys_size * sizeof(struct netdev_flow_key));
>          DP_PACKET_BATCH_FOR_EACH (i, packet, packets) {
> +            /* Ensure offsets is set by the opt impl. */
>              dp_packet_reset_offsets(packet);
> +            /* Ensure packet hash is re-calculated by opt impl. */
> +            dp_packet_reset_offload(packet);
>          }
>          /* Call optimized miniflow for each batch of packet. */
>          uint32_t hit_mask = mfex_impls[j].extract_func(packets, test_keys,
> @@ -303,6 +313,15 @@ dpif_miniflow_extract_autovalidator(struct
> dp_packet_batch *packets,
>                  failed = 1;
>              }
> 
> +            /* Check hashes are equal. */
> +            if ((keys[i].hash != test_keys[i].hash) ||
> +                (keys[i].len != test_keys[i].len)) {
> +                ds_put_format(&log_msg, "Good hash: %d len: %d\tTest hash:%d"
> +                              " len:%d\n", keys[i].hash, keys[i].len,
> +                              test_keys[i].hash, test_keys[i].len);
> +                failed = 1;
> +            }
> +
>              if (!miniflow_equal(&keys[i].mf, &test_keys[i].mf)) {
>                  uint32_t block_cnt = miniflow_n_values(&keys[i].mf);
>                  uint32_t test_block_cnt = miniflow_n_values(&test_keys[i].mf);
> --
> 2.25.1

> -----Original Message-----
> From: Stokes, Ian <ian.stokes@intel.com>
> Sent: Wednesday, January 12, 2022 9:47 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; ovs-
> dev@openvswitch.org
> Cc: i.maximets@ovn.org; Amber, Kumar <kumar.amber@intel.com>
> Subject: RE: [PATCH v5 2/2] dpif-netdev/mfex: Optimize packet hash and enable
> autovalidator
> 
> > From: Kumar Amber <kumar.amber@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.amber@intel.com>
> > Signed-off-by: Harry van Haaren <harry.van.haaren@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>