[ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when EMC is disabled.
Fischetti, Antonio
antonio.fischetti at intel.com
Wed Jul 19 15:58:39 UTC 2017
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 at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Fischetti, Antonio
> Sent: Friday, June 23, 2017 10:53 PM
> To: O Mahony, Billy <billy.o.mahony at intel.com>; dev at 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 at intel.com>;
> dev at 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 at intel.com>; dev at 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 at intel.com>;
> > > > dev at 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 at openvswitch.org [mailto:ovs-dev-
> > > > > bounces at openvswitch.org] On Behalf Of antonio.fischetti at intel.com
> > > > > Sent: Monday, June 19, 2017 11:12 AM
> > > > > To: dev at openvswitch.org
> > > > > Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash
> > > > > when EMC is disabled.
> > > > >
> > > > > From: Antonio Fischetti <antonio.fischetti at 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 at 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 at openvswitch.org
> > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list