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.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,
>
> > -----Original Message-----
> > From: Fischetti, Antonio
> > Sent: Friday, June 23, 2017 3:10 PM
> > To: O Mahony, Billy <billy.o.mah...@intel.com>; d...@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.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
