| Message ID | 1500480297-7530-2-git-send-email-antonio.fischetti@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Darrell Ball |
| Headers | show |
Hi Antonio, This is patch is definitely simpler than the original. However on the original patch I suggested: "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." I know you checked this and reported the performance gain was lower than with the v1 patch. We surmised that it was related to introducing a branch in the dpif_netdev_packet_get_rss_hash(). However there are many branches in this patch also. Can you give details of how you are testing? * What is the traffic * the flows/rules and * how are you measuring the performance difference (ie. cycles per packet or packet throughput or some other measure). Apologies for going on about this but if we can't get the same effect with a two or three line change than a 20line change I think it'll be worth it. One other comment below Thanks, Billy. > -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com > Sent: Wednesday, July 19, 2017 5:05 PM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH v2 2/5] dpif-netdev: Avoid reading RSS hash when > EMC is disabled. > > When EMC is disabled the reading of RSS hash is skipped. [[BO'M]] I think this is already the case with the existing code? Just addition of OVS_UNLIKELY on the check. > 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@intel.com> > --- > lib/dpif-netdev.c | 42 ++++++++++++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 8 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 123e04a..9562827 > 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -4472,6 +4472,22 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread > *pmd, struct dp_packet *packet_, } > > static inline uint32_t > +dpif_netdev_packet_get_rss_hash_orig_pkt(struct dp_packet *packet, > + const struct miniflow *mf) { > + uint32_t hash; > + > + if (OVS_LIKELY(dp_packet_rss_valid(packet))) { > + hash = dp_packet_get_rss_hash(packet); > + } else { > + hash = miniflow_hash_5tuple(mf, 0); > + dp_packet_set_rss_hash(packet, hash); > + } > + > + return hash; > +} > + > +static inline uint32_t > dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > const struct miniflow *mf) { @@ -4572,7 +4588,8 @@ static > inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, > struct dp_packet_batch *packets_, > struct netdev_flow_key *keys, > - struct packet_batch_per_flow batches[], size_t *n_batches) > + struct packet_batch_per_flow batches[], size_t *n_batches, > + bool md_is_valid) > { > struct emc_cache *flow_cache = &pmd->flow_cache; > struct netdev_flow_key *key = &keys[0]; @@ -4602,10 +4619,19 @@ > 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 EMC is disabled skip emc_lookup */ > - flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); > + /* If EMC is disabled skip hash computation and emc_lookup */ > + if (OVS_LIKELY(cur_min)) { > + if (!md_is_valid) { > + key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, > + &key->mf); > + } else { > + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key- > >mf); > + } > + flow = emc_lookup(flow_cache, key); > + } else { > + flow = NULL; > + } > if (OVS_LIKELY(flow)) { > dp_netdev_queue_batches(packet, flow, &key->mf, batches, > n_batches); @@ -4801,7 +4827,7 @@ > fast_path_processing(struct dp_netdev_pmd_thread *pmd, > * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ static > void dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > - struct dp_packet_batch *packets) > + struct dp_packet_batch *packets, bool md_is_valid) > { > int cnt = packets->count; > #if !defined(__CHECKER__) && !defined(_WIN32) @@ -4818,7 +4844,7 @@ > dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > odp_port_t in_port; > > n_batches = 0; > - emc_processing(pmd, packets, keys, batches, &n_batches); > + emc_processing(pmd, packets, keys, batches, &n_batches, > + md_is_valid); > if (!dp_packet_batch_is_empty(packets)) { > /* Get ingress port from first packet's metadata. */ > in_port = packets->packets[0]->md.in_port.odp_port; > @@ -4855,14 +4881,14 @@ dp_netdev_input(struct > dp_netdev_pmd_thread *pmd, > pkt_metadata_init(&packet->md, port_no); > } > > - dp_netdev_input__(pmd, packets); > + dp_netdev_input__(pmd, packets, false); > } > > static void > dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, > struct dp_packet_batch *packets) { > - dp_netdev_input__(pmd, packets); > + dp_netdev_input__(pmd, packets, true); > } > > struct dp_netdev_execute_aux { > -- > 2.4.11 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> -----Original Message----- > From: O Mahony, Billy > Sent: Monday, July 31, 2017 5:22 PM > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org > Subject: RE: [ovs-dev] [PATCH v2 2/5] dpif-netdev: Avoid reading RSS hash when > EMC is disabled. > > Hi Antonio, > > This is patch is definitely simpler than the original. > > However on the original patch I suggested: > > "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." > > I know you checked this and reported the performance gain was lower than with > the v1 patch. We surmised that it was related to introducing a branch in the > dpif_netdev_packet_get_rss_hash(). However there are many branches in this > patch also. > > Can you give details of how you are testing? > * What is the traffic > * the flows/rules and > * how are you measuring the performance difference (ie. cycles per packet or > packet throughput or some other measure). [Antonio] I'm using a port-to-port setup, sending 1 UDP flow, 64B packets. 2 PMDs with 3 Tx queues. The biggest difference is with case A where the 5-tuple hash is computed in software. Case A) RSS Hash is disabled. I see for each side the Rx pkt rate: Orig OvS + previous patch#1: 1.28 1.29 = 2.57 Mpps Orig OvS + previous patch#1 + this patch: 1.47 1.49 = 2.96 Mpps Case B) In case RSS Hash is enabled I see more or less the same performance: Orig OvS + previous patch#1: 11.59 11.72 = 23.31 Mpps Orig OvS + previous patch#1 + this patch: 11.45 11.84 = 23.29 Mpps Case C) RSS Hash is enabled, EMC is disabled. Orig OvS + previous patch#1 + No EMC: 7.68 7.65 = 15.33 Mpps Orig OvS + previous patch#1 + this patch: 7.62 7.73 = 15.35 Mpps > > Apologies for going on about this but if we can't get the same effect with a > two or three line change than a 20line change I think it'll be worth it. > > One other comment below > > Thanks, > Billy. > > > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of antonio.fischetti@intel.com > > Sent: Wednesday, July 19, 2017 5:05 PM > > To: dev@openvswitch.org > > Subject: [ovs-dev] [PATCH v2 2/5] dpif-netdev: Avoid reading RSS hash when > > EMC is disabled. > > > > When EMC is disabled the reading of RSS hash is skipped. > > [[BO'M]] I think this is already the case with the existing code? Just > addition of OVS_UNLIKELY on the check. [Antonio] No, in the existing code when EMC is disabled it just skips emc_lookup. miniflow_extract(packet, &key->mf); key->len = 0; /* Not computed yet. */ key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); <--- Read Hash /* If EMC is disabled skip emc_lookup */ flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); <--- Skip emc_lookup > > > 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@intel.com> > > --- > > lib/dpif-netdev.c | 42 ++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 34 insertions(+), 8 deletions(-) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 123e04a..9562827 > > 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -4472,6 +4472,22 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread > > *pmd, struct dp_packet *packet_, } > > > > static inline uint32_t > > +dpif_netdev_packet_get_rss_hash_orig_pkt(struct dp_packet *packet, > > + const struct miniflow *mf) { > > + uint32_t hash; > > + > > + if (OVS_LIKELY(dp_packet_rss_valid(packet))) { > > + hash = dp_packet_get_rss_hash(packet); > > + } else { > > + hash = miniflow_hash_5tuple(mf, 0); > > + dp_packet_set_rss_hash(packet, hash); > > + } > > + > > + return hash; > > +} > > + > > +static inline uint32_t > > dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, > > const struct miniflow *mf) { @@ -4572,7 > +4588,8 @@ static > > inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, > > struct dp_packet_batch *packets_, > > struct netdev_flow_key *keys, > > - struct packet_batch_per_flow batches[], size_t *n_batches) > > + struct packet_batch_per_flow batches[], size_t *n_batches, > > + bool md_is_valid) > > { > > struct emc_cache *flow_cache = &pmd->flow_cache; > > struct netdev_flow_key *key = &keys[0]; @@ -4602,10 +4619,19 @@ > > 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 EMC is disabled skip emc_lookup */ > > - flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); > > + /* If EMC is disabled skip hash computation and emc_lookup */ > > + if (OVS_LIKELY(cur_min)) { > > + if (!md_is_valid) { > > + key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, > > + &key->mf); > > + } else { > > + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key- > > >mf); > > + } > > + flow = emc_lookup(flow_cache, key); > > + } else { > > + flow = NULL; > > + } > > if (OVS_LIKELY(flow)) { > > dp_netdev_queue_batches(packet, flow, &key->mf, batches, > > n_batches); @@ -4801,7 +4827,7 @@ > > fast_path_processing(struct dp_netdev_pmd_thread *pmd, > > * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ > static > > void dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > > - struct dp_packet_batch *packets) > > + struct dp_packet_batch *packets, bool md_is_valid) > > { > > int cnt = packets->count; > > #if !defined(__CHECKER__) && !defined(_WIN32) @@ -4818,7 +4844,7 @@ > > dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, > > odp_port_t in_port; > > > > n_batches = 0; > > - emc_processing(pmd, packets, keys, batches, &n_batches); > > + emc_processing(pmd, packets, keys, batches, &n_batches, > > + md_is_valid); > > if (!dp_packet_batch_is_empty(packets)) { > > /* Get ingress port from first packet's metadata. */ > > in_port = packets->packets[0]->md.in_port.odp_port; > > @@ -4855,14 +4881,14 @@ dp_netdev_input(struct > > dp_netdev_pmd_thread *pmd, > > pkt_metadata_init(&packet->md, port_no); > > } > > > > - dp_netdev_input__(pmd, packets); > > + dp_netdev_input__(pmd, packets, false); > > } > > > > static void > > dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, > > struct dp_packet_batch *packets) { > > - dp_netdev_input__(pmd, packets); > > + dp_netdev_input__(pmd, packets, true); > > } > > > > struct dp_netdev_execute_aux { > > -- > > 2.4.11 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 123e04a..9562827 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4472,6 +4472,22 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, } static inline uint32_t +dpif_netdev_packet_get_rss_hash_orig_pkt(struct dp_packet *packet, + const struct miniflow *mf) +{ + uint32_t hash; + + if (OVS_LIKELY(dp_packet_rss_valid(packet))) { + hash = dp_packet_get_rss_hash(packet); + } else { + hash = miniflow_hash_5tuple(mf, 0); + dp_packet_set_rss_hash(packet, hash); + } + + return hash; +} + +static inline uint32_t dpif_netdev_packet_get_rss_hash(struct dp_packet *packet, const struct miniflow *mf) { @@ -4572,7 +4588,8 @@ static inline size_t emc_processing(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets_, struct netdev_flow_key *keys, - struct packet_batch_per_flow batches[], size_t *n_batches) + struct packet_batch_per_flow batches[], size_t *n_batches, + bool md_is_valid) { struct emc_cache *flow_cache = &pmd->flow_cache; struct netdev_flow_key *key = &keys[0]; @@ -4602,10 +4619,19 @@ 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 EMC is disabled skip emc_lookup */ - flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key); + /* If EMC is disabled skip hash computation and emc_lookup */ + if (OVS_LIKELY(cur_min)) { + if (!md_is_valid) { + key->hash = dpif_netdev_packet_get_rss_hash_orig_pkt(packet, + &key->mf); + } else { + key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf); + } + flow = emc_lookup(flow_cache, key); + } else { + flow = NULL; + } if (OVS_LIKELY(flow)) { dp_netdev_queue_batches(packet, flow, &key->mf, batches, n_batches); @@ -4801,7 +4827,7 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd, * valid, 'md_is_valid' must be true and 'port_no' will be ignored. */ static void dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, - struct dp_packet_batch *packets) + struct dp_packet_batch *packets, bool md_is_valid) { int cnt = packets->count; #if !defined(__CHECKER__) && !defined(_WIN32) @@ -4818,7 +4844,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd, odp_port_t in_port; n_batches = 0; - emc_processing(pmd, packets, keys, batches, &n_batches); + emc_processing(pmd, packets, keys, batches, &n_batches, md_is_valid); if (!dp_packet_batch_is_empty(packets)) { /* Get ingress port from first packet's metadata. */ in_port = packets->packets[0]->md.in_port.odp_port; @@ -4855,14 +4881,14 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd, pkt_metadata_init(&packet->md, port_no); } - dp_netdev_input__(pmd, packets); + dp_netdev_input__(pmd, packets, false); } static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd, struct dp_packet_batch *packets) { - dp_netdev_input__(pmd, packets); + dp_netdev_input__(pmd, packets, true); } struct dp_netdev_execute_aux {
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@intel.com> --- lib/dpif-netdev.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)