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