diff mbox

[ovs-dev,1/4] dpif-netdev: Avoid reading RSS hash when EMC is disabled.

Message ID 1497867118-4195-1-git-send-email-antonio.fischetti@intel.com
State Superseded
Headers show

Commit Message

Fischetti, Antonio June 19, 2017, 10:11 a.m. UTC 
From: Antonio Fischetti <antonio.fischetti@intel.com>

When EMC is disabled the reading of RSS hash is skipped.
For packets that are not recirculated it retrieves the hash
value without considering the recirc id.

This is mostly a preliminary change for the next patch in
this series.

Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
In our testbench we used monodirectional traffic with
64B UDP packets
PDM threads:  2
Traffic gen. streams: 1

we saw the following performance improvement:

Orig               11.49 Mpps
With Patch#1:      11.62 Mpps

 lib/dpif-netdev.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Comments

Billy O'Mahony June 23, 2017, 12:53 p.m. UTC | #1 
Hi Antonio,

In this patch of the patchset there are three lines removed from the direct command flow:

-        miniflow_extract(packet, &key->mf);
-        key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
-        flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);

Which are then replicated in several different branches for logic. This is a lot of duplication of logic. 

I *think* (I haven't tested it) this can be re-written with less branching like this:

         if (!md_is_valid) {
             pkt_metadata_init(&packet->md, port_no);
         }
         miniflow_extract(packet, &key->mf);
         if (OVS_LIKELY(cur_min)) {
             if (md_is_valid) {
                     key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
                 }
                 else
                 {
                     if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
                         key->hash = dp_packet_get_rss_hash(packet);
                     } else {
                         key->hash = miniflow_hash_5tuple(&key->mf, 0);
                         dp_packet_set_rss_hash(packet, key->hash);
                     }
                 flow = emc_lookup(flow_cache, key);
                 }
         } else {
             flow = NULL;
         }

Also if I'm understanding correctly the final effect of the patch is that in the case where !md_is_valid it effectively replicates the work of dpif_netdev_packet_get_rss_hash() but leaving out the if (recirc_depth) block of that fn. This is effectively overriding the return value of recirc_depth_get_unsafe in dpif_netdev_packet_get_rss_hash() and forcing/assuming that it is zero. 

If so it would be less disturbing to the existing code to just add a bool arg to dpif_netdev_packet_get_rss_hash() called do_not_check_recirc_depth and use that to return early (before the if (recirc_depth) check). Also in that case the patch would require none of the  conditional logic changes (neither the original or that suggested in this email) and should be able to just set the proposed do_not_check_recirc_depth based on md_is_valid.

Also this is showing up as a patch set can you add a cover letter to outline the overall goal of the patchset.

Thanks,
Billy. 


> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com
> Sent: Monday, June 19, 2017 11:12 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when
> EMC is disabled.
> 
> From: Antonio Fischetti <antonio.fischetti@intel.com>
> 
> When EMC is disabled the reading of RSS hash is skipped.
> For packets that are not recirculated it retrieves the hash value without
> considering the recirc id.
> 
> This is mostly a preliminary change for the next patch in this series.
> 
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
> In our testbench we used monodirectional traffic with 64B UDP packets PDM
> threads:  2 Traffic gen. streams: 1
> 
> we saw the following performance improvement:
> 
> Orig               11.49 Mpps
> With Patch#1:      11.62 Mpps
> 
>  lib/dpif-netdev.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 02af32e..fd2ed52
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4584,13 +4584,33 @@ emc_processing(struct dp_netdev_pmd_thread
> *pmd,
> 
>          if (!md_is_valid) {
>              pkt_metadata_init(&packet->md, port_no);
> +            miniflow_extract(packet, &key->mf);
> +            /* This is not a recirculated packet. */
> +            if (OVS_LIKELY(cur_min)) {
> +                /* EMC is enabled.  We can retrieve the 5-tuple hash
> +                 * without considering the recirc id. */
> +                if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
> +                    key->hash = dp_packet_get_rss_hash(packet);
> +                } else {
> +                    key->hash = miniflow_hash_5tuple(&key->mf, 0);
> +                    dp_packet_set_rss_hash(packet, key->hash);
> +                }
> +                flow = emc_lookup(flow_cache, key);
> +            } else {
> +                /* EMC is disabled, skip emc_lookup. */
> +                flow = NULL;
> +            }
> +        } else {
> +            /* Recirculated packets. */
> +            miniflow_extract(packet, &key->mf);
> +            if (OVS_LIKELY(cur_min)) {
> +                key->hash = dpif_netdev_packet_get_rss_hash(packet, &key-
> >mf);
> +                flow = emc_lookup(flow_cache, key);
> +            } else {
> +                flow = NULL;
> +            }
>          }
> -        miniflow_extract(packet, &key->mf);
>          key->len = 0; /* Not computed yet. */
> -        key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
> -
> -        /* If EMC is disabled skip emc_lookup */
> -        flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
>          if (OVS_LIKELY(flow)) {
>              dp_netdev_queue_batches(packet, flow, &key->mf, batches,
>                                      n_batches);
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Fischetti, Antonio June 23, 2017, 2:10 p.m. UTC | #2 
Hi Billy,
thanks a lot for you suggestions. Those would really help re-factoring
the code by avoiding duplications.
The thing is that this patch 1/4 is mainly a preparation for the 
next patch 2/4. So I did these changes with the next patch 2/4 in mind.

The final result I meant to achieve in patch 2/4 is the following.
EMC lookup is skipped - not only when EMC is disabled - but also when
(we're processing recirculated packets) && (the EMC is 'enough' full).
The purpose is to avoid EMC thrashing.

Below is how the code looks like after applying patches 1/4 and 2/4.
Please let me know if you can find some similar optimizations to 
avoid code duplications, that would be great.
========================
        /*
         * EMC lookup is skipped when one or both of the following
         * two cases occurs:
         *
         *   - EMC is disabled.  This is detected from cur_min.
         *
         *   - The EMC occupancy exceeds EMC_FULL_THRESHOLD and the
         *     packet to be classified is being recirculated.  When this
         *     happens also EMC insertions are skipped for recirculated
         *     packets.  So that EMC is used just to store entries which
         *     are hit from the 'original' packets.  This way the EMC
         *     thrashing is mitigated with a benefit on performance.
         */
        if (!md_is_valid) {
            pkt_metadata_init(&packet->md, port_no);
            miniflow_extract(packet, &key->mf);  <== this fn must be called after pkt_metadta_init
            /* This is not a recirculated packet. */
            if (OVS_LIKELY(cur_min)) {
                /* EMC is enabled.  We can retrieve the 5-tuple hash
                 * without considering the recirc id. */
                if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
                    key->hash = dp_packet_get_rss_hash(packet);
                } else {
                    key->hash = miniflow_hash_5tuple(&key->mf, 0);
                    dp_packet_set_rss_hash(packet, key->hash);
                }
                flow = emc_lookup(flow_cache, key);
            } else {
                /* EMC is disabled, skip emc_lookup. */
                flow = NULL;
            }
        } else {
            /* Recirculated packets. */
            miniflow_extract(packet, &key->mf);
            if (flow_cache->n_entries & EMC_FULL_THRESHOLD) {
                /* EMC occupancy is over the threshold.  We skip EMC
                 * lookup for recirculated packets. */
                flow = NULL;
            } else {
                if (OVS_LIKELY(cur_min)) {
                    key->hash = dpif_netdev_packet_get_rss_hash(packet,
                                    &key->mf);
                    flow = emc_lookup(flow_cache, key);
                } else {
                    flow = NULL;
                }
            }
        }
================================

Basically patch 1/4 is mostly a preliminary change for 2/4.

Yes, patch 1/4 also allows to avoid reading hash when EMC is disabled.
Or - for packets that are not recirculated - avoids calling
recirc_depth_get_unsafe() when reading the hash.

Also, as these functions are critical for performance, I tend to avoid
adding new Booleans that require new if statements.


Thanks,
Antonio

> -----Original Message-----
> From: O Mahony, Billy
> Sent: Friday, June 23, 2017 1:54 PM
> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash
> when EMC is disabled.
> 
> Hi Antonio,
> 
> In this patch of the patchset there are three lines removed from the
> direct command flow:
> 
> -        miniflow_extract(packet, &key->mf);
> -        key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
> -        flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
> 
> Which are then replicated in several different branches for logic. This is
> a lot of duplication of logic.
> 
> I *think* (I haven't tested it) this can be re-written with less branching
> like this:
> 
>          if (!md_is_valid) {
>              pkt_metadata_init(&packet->md, port_no);
>          }
>          miniflow_extract(packet, &key->mf);
>          if (OVS_LIKELY(cur_min)) {
>              if (md_is_valid) {
>                      key->hash = dpif_netdev_packet_get_rss_hash(packet,
> &key->mf);
>                  }
>                  else
>                  {
>                      if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
>                          key->hash = dp_packet_get_rss_hash(packet);
>                      } else {
>                          key->hash = miniflow_hash_5tuple(&key->mf, 0);
>                          dp_packet_set_rss_hash(packet, key->hash);
>                      }
>                  flow = emc_lookup(flow_cache, key);
>                  }
>          } else {
>              flow = NULL;
>          }
> 
> Also if I'm understanding correctly the final effect of the patch is that
> in the case where !md_is_valid it effectively replicates the work of
> dpif_netdev_packet_get_rss_hash() but leaving out the if (recirc_depth)
> block of that fn. This is effectively overriding the return value of
> recirc_depth_get_unsafe in dpif_netdev_packet_get_rss_hash() and
> forcing/assuming that it is zero.
> 
> If so it would be less disturbing to the existing code to just add a bool
> arg to dpif_netdev_packet_get_rss_hash() called do_not_check_recirc_depth
> and use that to return early (before the if (recirc_depth) check). Also in
> that case the patch would require none of the  conditional logic changes
> (neither the original or that suggested in this email) and should be able
> to just set the proposed do_not_check_recirc_depth based on md_is_valid.
> 
> Also this is showing up as a patch set can you add a cover letter to
> outline the overall goal of the patchset.
> 
> Thanks,
> Billy.
> 
> 
> > -----Original Message-----
> > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com
> > Sent: Monday, June 19, 2017 11:12 AM
> > To: dev@openvswitch.org
> > Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when
> > EMC is disabled.
> >
> > From: Antonio Fischetti <antonio.fischetti@intel.com>
> >
> > When EMC is disabled the reading of RSS hash is skipped.
> > For packets that are not recirculated it retrieves the hash value
> without
> > considering the recirc id.
> >
> > This is mostly a preliminary change for the next patch in this series.
> >
> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> > ---
> > In our testbench we used monodirectional traffic with 64B UDP packets
> PDM
> > threads:  2 Traffic gen. streams: 1
> >
> > we saw the following performance improvement:
> >
> > Orig               11.49 Mpps
> > With Patch#1:      11.62 Mpps
> >
> >  lib/dpif-netdev.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> 02af32e..fd2ed52
> > 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4584,13 +4584,33 @@ emc_processing(struct dp_netdev_pmd_thread
> > *pmd,
> >
> >          if (!md_is_valid) {
> >              pkt_metadata_init(&packet->md, port_no);
> > +            miniflow_extract(packet, &key->mf);
> > +            /* This is not a recirculated packet. */
> > +            if (OVS_LIKELY(cur_min)) {
> > +                /* EMC is enabled.  We can retrieve the 5-tuple hash
> > +                 * without considering the recirc id. */
> > +                if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
> > +                    key->hash = dp_packet_get_rss_hash(packet);
> > +                } else {
> > +                    key->hash = miniflow_hash_5tuple(&key->mf, 0);
> > +                    dp_packet_set_rss_hash(packet, key->hash);
> > +                }
> > +                flow = emc_lookup(flow_cache, key);
> > +            } else {
> > +                /* EMC is disabled, skip emc_lookup. */
> > +                flow = NULL;
> > +            }
> > +        } else {
> > +            /* Recirculated packets. */
> > +            miniflow_extract(packet, &key->mf);
> > +            if (OVS_LIKELY(cur_min)) {
> > +                key->hash = dpif_netdev_packet_get_rss_hash(packet,
> &key-
> > >mf);
> > +                flow = emc_lookup(flow_cache, key);
> > +            } else {
> > +                flow = NULL;
> > +            }
> >          }
> > -        miniflow_extract(packet, &key->mf);
> >          key->len = 0; /* Not computed yet. */
> > -        key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
> > -
> > -        /* If EMC is disabled skip emc_lookup */
> > -        flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
> >          if (OVS_LIKELY(flow)) {
> >              dp_netdev_queue_batches(packet, flow, &key->mf, batches,
> >                                      n_batches);
> > --
> > 2.4.11
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 02af32e..fd2ed52 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4584,13 +4584,33 @@  emc_processing(struct dp_netdev_pmd_thread *pmd,
 
         if (!md_is_valid) {
             pkt_metadata_init(&packet->md, port_no);
+            miniflow_extract(packet, &key->mf);
+            /* This is not a recirculated packet. */
+            if (OVS_LIKELY(cur_min)) {
+                /* EMC is enabled.  We can retrieve the 5-tuple hash
+                 * without considering the recirc id. */
+                if (OVS_LIKELY(dp_packet_rss_valid(packet))) {
+                    key->hash = dp_packet_get_rss_hash(packet);
+                } else {
+                    key->hash = miniflow_hash_5tuple(&key->mf, 0);
+                    dp_packet_set_rss_hash(packet, key->hash);
+                }
+                flow = emc_lookup(flow_cache, key);
+            } else {
+                /* EMC is disabled, skip emc_lookup. */
+                flow = NULL;
+            }
+        } else {
+            /* Recirculated packets. */
+            miniflow_extract(packet, &key->mf);
+            if (OVS_LIKELY(cur_min)) {
+                key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
+                flow = emc_lookup(flow_cache, key);
+            } else {
+                flow = NULL;
+            }
         }
-        miniflow_extract(packet, &key->mf);
         key->len = 0; /* Not computed yet. */
-        key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
-
-        /* If EMC is disabled skip emc_lookup */
-        flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
         if (OVS_LIKELY(flow)) {
             dp_netdev_queue_batches(packet, flow, &key->mf, batches,
                                     n_batches);