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.fische...@intel.com>; d...@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-boun...@openvswitch.org [mailto:ovs-dev- > > boun...@openvswitch.org] On Behalf Of antonio.fische...@intel.com > > Sent: Monday, June 19, 2017 11:12 AM > > To: d...@openvswitch.org > > Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when > > EMC is disabled. > > > > From: Antonio Fischetti <antonio.fische...@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.fische...@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 > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev