Hi Antonio,
> -----Original Message-----
> From: Fischetti, Antonio
> Sent: Friday, June 23, 2017 3:10 PM
> To: O Mahony, Billy <[email protected]>; [email protected]
> 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 <[email protected]>;
> > [email protected]
> > 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: [email protected] [mailto:ovs-dev-
> > > [email protected]] On Behalf Of [email protected]
> > > Sent: Monday, June 19, 2017 11:12 AM
> > > To: [email protected]
> > > Subject: [ovs-dev] [PATCH 1/4] dpif-netdev: Avoid reading RSS hash
> > > when EMC is disabled.
> > >
> > > From: Antonio Fischetti <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
