| Message ID | 03135AEA779D444E90975C2703F148DC58BD91E4@IRSMSX107.ger.corp.intel.com |
|---|---|
| State | Deferred |
| Headers | show |
Hi Billy, thanks for your suggestion, it makes the code more clean and readable. Once I get back from vacation I'll give it a try and check if this still gives a performance benefit. /Antonio > -----Original Message----- > From: O Mahony, Billy > Sent: Friday, June 23, 2017 5:23 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, > > > -----Original Message----- > > From: Fischetti, Antonio > > Sent: Friday, June 23, 2017 3:10 PM > > To: O Mahony, Billy <billy.o.mahony@intel.com>; dev@openvswitch.org > > Subject: RE: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash > > when EMC is disabled. > > > > 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. > [[BO'M]] > > Can you investigate refactoring this patch with something like below. I > think this is equivalent. The current patch duplicates miniflow_extract, > emc_lookup across the md_is_valid and !md_is_valid branches. It also > duplicates some of the internals of get_rss_hash out into the !md_is_valid > case and is difficult to follow. > > If the following suggestion works the change in emc_processing from patch > 2/4 can easily be grafted on to that. > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 4e29085..a7e854d 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4442,7 +4442,8 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, > struct dp_packet *packet_, > > static inline uint32_t > dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > - const struct miniflow *mf) > + const struct miniflow *mf, > + bool use_recirc_depth) > { > uint32_t hash, recirc_depth; > > @@ -4456,7 +4457,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet > *packet, > /* The RSS hash must account for the recirculation depth to avoid > * collisions in the exact match cache */ > recirc_depth = *recirc_depth_get_unsafe(); > - if (OVS_UNLIKELY(recirc_depth)) { > + if (OVS_UNLIKELY(use_recirc_depth && recirc_depth)) { > hash = hash_finish(hash, recirc_depth); > dp_packet_set_rss_hash(packet, hash); > } > @@ -4575,7 +4576,13 @@ emc_processing(struct dp_netdev_pmd_thread *pmd, > } > miniflow_extract(packet, &key->mf); > key->len = 0; /* Not computed yet. */ > - key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); > + if (cur_min) { > + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf, > md_is_valid); > + flow = emc_lookup(flow_cache, key); > + } > + else { > + flow = NULL; > + } > > /* If EMC is disabled skip emc_lookup */ > flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); > > > > > > 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
Hi Billy, your suggestion really simplify the code a lot and improve readability but unfortunately there's no gain in performance. Anyway in the next version I'm adding some further change and I will try to take into account your suggestions. /Antonio > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Fischetti, Antonio > Sent: Friday, June 23, 2017 10:53 PM > To: O Mahony, Billy <billy.o.mahony@intel.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash > when EMC is disabled. > > Hi Billy, thanks for your suggestion, it makes the code more clean > and readable. > Once I get back from vacation I'll give it a try and check if this > still gives a performance benefit. > > /Antonio > > > -----Original Message----- > > From: O Mahony, Billy > > Sent: Friday, June 23, 2017 5:23 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, > > > > > -----Original Message----- > > > From: Fischetti, Antonio > > > Sent: Friday, June 23, 2017 3:10 PM > > > To: O Mahony, Billy <billy.o.mahony@intel.com>; dev@openvswitch.org > > > Subject: RE: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash > > > when EMC is disabled. > > > > > > 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. > > [[BO'M]] > > > > Can you investigate refactoring this patch with something like below. I > > think this is equivalent. The current patch duplicates > miniflow_extract, > > emc_lookup across the md_is_valid and !md_is_valid branches. It also > > duplicates some of the internals of get_rss_hash out into the > !md_is_valid > > case and is difficult to follow. > > > > If the following suggestion works the change in emc_processing from > patch > > 2/4 can easily be grafted on to that. > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 4e29085..a7e854d 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -4442,7 +4442,8 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, > > struct dp_packet *packet_, > > > > static inline uint32_t > > dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > > - const struct miniflow *mf) > > + const struct miniflow *mf, > > + bool use_recirc_depth) > > { > > uint32_t hash, recirc_depth; > > > > @@ -4456,7 +4457,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet > > *packet, > > /* The RSS hash must account for the recirculation depth to avoid > > * collisions in the exact match cache */ > > recirc_depth = *recirc_depth_get_unsafe(); > > - if (OVS_UNLIKELY(recirc_depth)) { > > + if (OVS_UNLIKELY(use_recirc_depth && recirc_depth)) { > > hash = hash_finish(hash, recirc_depth); > > dp_packet_set_rss_hash(packet, hash); > > } > > @@ -4575,7 +4576,13 @@ emc_processing(struct dp_netdev_pmd_thread *pmd, > > } > > miniflow_extract(packet, &key->mf); > > key->len = 0; /* Not computed yet. */ > > - key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); > > + if (cur_min) { > > + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key- > >mf, > > md_is_valid); > > + flow = emc_lookup(flow_cache, key); > > + } > > + else { > > + flow = NULL; > > + } > > > > /* If EMC is disabled skip emc_lookup */ > > flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); > > > > > > > > > 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 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Antonio, Ok, I guess you can't argue with performance! I look forward to the next rev. No further comments below. /Billy > -----Original Message----- > From: Fischetti, Antonio > Sent: Wednesday, July 19, 2017 4:59 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; O Mahony, Billy > <billy.o.mahony@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash > when EMC is disabled. > > Hi Billy, your suggestion really simplify the code a lot and improve readability > but unfortunately there's no gain in performance. > Anyway in the next version I'm adding some further change and I will try to > take into account your suggestions. > > /Antonio > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Fischetti, Antonio > > Sent: Friday, June 23, 2017 10:53 PM > > To: O Mahony, Billy <billy.o.mahony@intel.com>; dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash > > when EMC is disabled. > > > > Hi Billy, thanks for your suggestion, it makes the code more clean and > > readable. > > Once I get back from vacation I'll give it a try and check if this > > still gives a performance benefit. > > > > /Antonio > > > > > -----Original Message----- > > > From: O Mahony, Billy > > > Sent: Friday, June 23, 2017 5:23 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, > > > > > > > -----Original Message----- > > > > From: Fischetti, Antonio > > > > Sent: Friday, June 23, 2017 3:10 PM > > > > To: O Mahony, Billy <billy.o.mahony@intel.com>; > > > > dev@openvswitch.org > > > > Subject: RE: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS > > > > hash when EMC is disabled. > > > > > > > > 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. > > > [[BO'M]] > > > > > > Can you investigate refactoring this patch with something like > > > below. I think this is equivalent. The current patch duplicates > > miniflow_extract, > > > emc_lookup across the md_is_valid and !md_is_valid branches. It also > > > duplicates some of the internals of get_rss_hash out into the > > !md_is_valid > > > case and is difficult to follow. > > > > > > If the following suggestion works the change in emc_processing from > > patch > > > 2/4 can easily be grafted on to that. > > > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > > > 4e29085..a7e854d 100644 > > > --- a/lib/dpif-netdev.c > > > +++ b/lib/dpif-netdev.c > > > @@ -4442,7 +4442,8 @@ dp_netdev_upcall(struct > dp_netdev_pmd_thread > > > *pmd, struct dp_packet *packet_, > > > > > > static inline uint32_t > > > dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > > > - const struct miniflow *mf) > > > + const struct miniflow *mf, > > > + bool use_recirc_depth) > > > { > > > uint32_t hash, recirc_depth; > > > > > > @@ -4456,7 +4457,7 @@ dpif_netdev_packet_get_rss_hash(struct > > > dp_packet *packet, > > > /* The RSS hash must account for the recirculation depth to avoid > > > * collisions in the exact match cache */ > > > recirc_depth = *recirc_depth_get_unsafe(); > > > - if (OVS_UNLIKELY(recirc_depth)) { > > > + if (OVS_UNLIKELY(use_recirc_depth && recirc_depth)) { > > > hash = hash_finish(hash, recirc_depth); > > > dp_packet_set_rss_hash(packet, hash); > > > } > > > @@ -4575,7 +4576,13 @@ emc_processing(struct > dp_netdev_pmd_thread *pmd, > > > } > > > miniflow_extract(packet, &key->mf); > > > key->len = 0; /* Not computed yet. */ > > > - key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); > > > + if (cur_min) { > > > + key->hash = dpif_netdev_packet_get_rss_hash(packet, > > > + &key- > > >mf, > > > md_is_valid); > > > + flow = emc_lookup(flow_cache, key); > > > + } > > > + else { > > > + flow = NULL; > > > + } > > > > > > /* If EMC is disabled skip emc_lookup */ > > > flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); > > > > > > > > > > > > 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 > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 4e29085..a7e854d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4442,7 +4442,8 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, static inline uint32_t dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, - const struct miniflow *mf) + const struct miniflow *mf, + bool use_recirc_depth) { uint32_t hash, recirc_depth; @@ -4456,7 +4457,7 @@ dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, /* The RSS hash must account for the recirculation depth to avoid * collisions in the exact match cache */ recirc_depth = *recirc_depth_get_unsafe(); - if (OVS_UNLIKELY(recirc_depth)) { + if (OVS_UNLIKELY(use_recirc_depth && recirc_depth)) { hash = hash_finish(hash, recirc_depth); dp_packet_set_rss_hash(packet, hash); } @@ -4575,7 +4576,13 @@ emc_processing(struct dp_netdev_pmd_thread *pmd, } miniflow_extract(packet, &key->mf); key->len = 0; /* Not computed yet. */ - key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); + if (cur_min) { + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf, md_is_valid); + flow = emc_lookup(flow_cache, key); + } + else { + flow = NULL; + } /* If EMC is disabled skip emc_lookup */ flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);