[ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash when EMC is disabled.
Fischetti, Antonio
antonio.fischetti at intel.com
Fri Jun 23 14:10:26 UTC 2017
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.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
More information about the dev
mailing list