| Message ID | 20190311163150.17063-1-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Fix double parsing of packets when EMC disabled. | expand |
On 11.03.2019 19:31, Ilya Maximets wrote: > This partially reverts commit bde94613e6276d48a6e0be7a592ebcf9836b4aaf. > > Commit bde94613e627 was aimed to slightly ( < 1%) increase performance > in the case where EMC disabled, but it avoids RSS hash calculation and > OVS has to calculate it while executing OVS_ACTION_ATTR_HASH in order > to handle balanced-tcp bonding. At the time of executing that action > there is no parsed flow, and OVS parses the packet for the second time > to calculate the hash. This happens for all packets received from the > virtual interfaces because they have no HW RSS. > > Here is the example of 'perf' output for VM --> (bonded PHY) traffic: > > Samples: 401K of event 'cycles', Event count (approx.): 50964771478 > Overhead Shared Object Symbol > 27.50% ovs-vswitchd [.] dpcls_lookup.370382 > 16.30% ovs-vswitchd [.] rte_vhost_dequeue_burst.9267 > 14.95% ovs-vswitchd [.] miniflow_extract > 7.22% ovs-vswitchd [.] flow_extract > 7.10% ovs-vswitchd [.] dp_netdev_input__.371002.4826 > 4.01% ovs-vswitchd [.] fast_path_processing.370987.4893 > > We can see that packet parsed twice. First time by 'miniflow_extract' > right after receiving and the second time by 'flow_extract' while > executing actions. > > In this particular case calculating RSS on receive saves > 7% of the > total CPU processing time. It varies from ~7 to ~10 % depending on > scenario/traffic types. > > It's better to calculate hash each time because performance > improvements of avoiding are negligible in compare with performance > drop in case of sending packets to bonded interface. > > Another solution could be to pass the parsed flow explicitly through > the datapath, but this will require big code changes and will have > additional overhead for metadata updating on packet changes. > > Also, this change should have small impact since SMC works well in most > cases and will be enabled/recommended by default in the future. > > CC: Antonio Fischetti <antonio.fischetti@intel.com> > Fixes: bde94613e627 ("dpif-netdev: Avoid reading RSS hash when EMC is disabled.") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- Hi. Any thoughts about this patch? This is a big performance issue on setups with OVS bonding. Best regards, Ilya Maximets. > lib/dpif-netdev.c | 21 +++++++-------------- > 1 file changed, 7 insertions(+), 14 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 4d6d0c372..35cd00712 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -6438,20 +6438,13 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, > > miniflow_extract(packet, &key->mf); > key->len = 0; /* Not computed yet. */ > - /* If EMC and SMC disabled skip hash computation */ > - if (smc_enable_db == true || cur_min != 0) { > - 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); > - } > - } > - if (cur_min) { > - flow = emc_lookup(&cache->emc_cache, key); > - } else { > - flow = NULL; > - } > + key->hash = > + (md_is_valid == false) > + ? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf) > + : dpif_netdev_packet_get_rss_hash(packet, &key->mf); > + > + /* If EMC is disabled skip emc_lookup */ > + flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL; > if (OVS_LIKELY(flow)) { > tcp_flags = miniflow_get_tcp_flags(&key->mf); > n_emc_hit++; >
On 4/18/2019 8:55 AM, Ilya Maximets wrote: > On 11.03.2019 19:31, Ilya Maximets wrote: >> This partially reverts commit bde94613e6276d48a6e0be7a592ebcf9836b4aaf. >> >> Commit bde94613e627 was aimed to slightly ( < 1%) increase performance >> in the case where EMC disabled, but it avoids RSS hash calculation and >> OVS has to calculate it while executing OVS_ACTION_ATTR_HASH in order >> to handle balanced-tcp bonding. At the time of executing that action >> there is no parsed flow, and OVS parses the packet for the second time >> to calculate the hash. This happens for all packets received from the >> virtual interfaces because they have no HW RSS. >> >> Here is the example of 'perf' output for VM --> (bonded PHY) traffic: >> >> Samples: 401K of event 'cycles', Event count (approx.): 50964771478 >> Overhead Shared Object Symbol >> 27.50% ovs-vswitchd [.] dpcls_lookup.370382 >> 16.30% ovs-vswitchd [.] rte_vhost_dequeue_burst.9267 >> 14.95% ovs-vswitchd [.] miniflow_extract >> 7.22% ovs-vswitchd [.] flow_extract >> 7.10% ovs-vswitchd [.] dp_netdev_input__.371002.4826 >> 4.01% ovs-vswitchd [.] fast_path_processing.370987.4893 >> >> We can see that packet parsed twice. First time by 'miniflow_extract' >> right after receiving and the second time by 'flow_extract' while >> executing actions. >> >> In this particular case calculating RSS on receive saves > 7% of the >> total CPU processing time. It varies from ~7 to ~10 % depending on >> scenario/traffic types. >> >> It's better to calculate hash each time because performance >> improvements of avoiding are negligible in compare with performance >> drop in case of sending packets to bonded interface. >> >> Another solution could be to pass the parsed flow explicitly through >> the datapath, but this will require big code changes and will have >> additional overhead for metadata updating on packet changes. >> >> Also, this change should have small impact since SMC works well in most >> cases and will be enabled/recommended by default in the future. >> >> CC: Antonio Fischetti <antonio.fischetti@intel.com> >> Fixes: bde94613e627 ("dpif-netdev: Avoid reading RSS hash when EMC is disabled.") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> --- > > Hi. > Any thoughts about this patch? > This is a big performance issue on setups with OVS bonding. > Hi Ilya, I'm just finishing up performance testing I started yesterday with this patch, I'm inclined to agree with you. From bench marking with master I see the following with EMC disabled with RFC2544 (0% packet loss). (Master - No hash when EMC disabled) Packet Size RX Rate PPs RX Rate Mbps RX % 64 6824334.554 3494.059 45.859 128 6878122.822 7043.198 81.438 256 4528973.86 9275.338 100 512 2349613.015 9624.015 100 1024 1197312.023 9808.38 100 1500 822368.143 9868.418 100 With the revert applied I see (Patch - hash applied when EMC disabled) Packet Size RX Rate PPs RX Rate Mbps RX % 64 6479041.962 3317.269 43.539 128 6747525.93 6909.467 79.891 256 4528894.363 9275.176 100 512 2349615.976 9624.027 100 1024 1197313.173 9808.39 100 1500 822365.834 9868.39 100 So the gain for avoiding the hash is marginal and certainly not equal to the gain achieved when enabled with bonding as you reported. Even testing with scaling high number of flows there is a similar pattern. (Master - No hash when EMC disabled) Packet Size RX Rate PPs RX Rate Mbps RX % 64 13418452.15 6870.248 45.086 (Patch - hash applied when EMC disabled) Packet Size RX Rate PPs RX Rate Mbps RX % 64 12958087.12 6634.541 43.539 With this in mind I'll push this to master today. Ian > Best regards, Ilya Maximets. > >> lib/dpif-netdev.c | 21 +++++++-------------- >> 1 file changed, 7 insertions(+), 14 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 4d6d0c372..35cd00712 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -6438,20 +6438,13 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, >> >> miniflow_extract(packet, &key->mf); >> key->len = 0; /* Not computed yet. */ >> - /* If EMC and SMC disabled skip hash computation */ >> - if (smc_enable_db == true || cur_min != 0) { >> - 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); >> - } >> - } >> - if (cur_min) { >> - flow = emc_lookup(&cache->emc_cache, key); >> - } else { >> - flow = NULL; >> - } >> + key->hash = >> + (md_is_valid == false) >> + ? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf) >> + : dpif_netdev_packet_get_rss_hash(packet, &key->mf); >> + >> + /* If EMC is disabled skip emc_lookup */ >> + flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL; >> if (OVS_LIKELY(flow)) { >> tcp_flags = miniflow_get_tcp_flags(&key->mf); >> n_emc_hit++; >>
On 18.04.2019 11:08, Ian Stokes wrote: > On 4/18/2019 8:55 AM, Ilya Maximets wrote: >> On 11.03.2019 19:31, Ilya Maximets wrote: >>> This partially reverts commit bde94613e6276d48a6e0be7a592ebcf9836b4aaf. >>> >>> Commit bde94613e627 was aimed to slightly ( < 1%) increase performance >>> in the case where EMC disabled, but it avoids RSS hash calculation and >>> OVS has to calculate it while executing OVS_ACTION_ATTR_HASH in order >>> to handle balanced-tcp bonding. At the time of executing that action >>> there is no parsed flow, and OVS parses the packet for the second time >>> to calculate the hash. This happens for all packets received from the >>> virtual interfaces because they have no HW RSS. >>> >>> Here is the example of 'perf' output for VM --> (bonded PHY) traffic: >>> >>> Samples: 401K of event 'cycles', Event count (approx.): 50964771478 >>> Overhead Shared Object Symbol >>> 27.50% ovs-vswitchd [.] dpcls_lookup.370382 >>> 16.30% ovs-vswitchd [.] rte_vhost_dequeue_burst.9267 >>> 14.95% ovs-vswitchd [.] miniflow_extract >>> 7.22% ovs-vswitchd [.] flow_extract >>> 7.10% ovs-vswitchd [.] dp_netdev_input__.371002.4826 >>> 4.01% ovs-vswitchd [.] fast_path_processing.370987.4893 >>> >>> We can see that packet parsed twice. First time by 'miniflow_extract' >>> right after receiving and the second time by 'flow_extract' while >>> executing actions. >>> >>> In this particular case calculating RSS on receive saves > 7% of the >>> total CPU processing time. It varies from ~7 to ~10 % depending on >>> scenario/traffic types. >>> >>> It's better to calculate hash each time because performance >>> improvements of avoiding are negligible in compare with performance >>> drop in case of sending packets to bonded interface. >>> >>> Another solution could be to pass the parsed flow explicitly through >>> the datapath, but this will require big code changes and will have >>> additional overhead for metadata updating on packet changes. >>> >>> Also, this change should have small impact since SMC works well in most >>> cases and will be enabled/recommended by default in the future. >>> >>> CC: Antonio Fischetti <antonio.fischetti@intel.com> >>> Fixes: bde94613e627 ("dpif-netdev: Avoid reading RSS hash when EMC is disabled.") >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> --- >> >> Hi. >> Any thoughts about this patch? >> This is a big performance issue on setups with OVS bonding. >> > > Hi Ilya, > > I'm just finishing up performance testing I started yesterday with this patch, I'm inclined to agree with you. > > From bench marking with master I see the following with EMC disabled with RFC2544 (0% packet loss). > > (Master - No hash when EMC disabled) > Packet Size RX Rate PPs RX Rate Mbps RX % > 64 6824334.554 3494.059 45.859 > 128 6878122.822 7043.198 81.438 > 256 4528973.86 9275.338 100 > 512 2349613.015 9624.015 100 > 1024 1197312.023 9808.38 100 > 1500 822368.143 9868.418 100 > > With the revert applied I see > > (Patch - hash applied when EMC disabled) > Packet Size RX Rate PPs RX Rate Mbps RX % > 64 6479041.962 3317.269 43.539 > 128 6747525.93 6909.467 79.891 > 256 4528894.363 9275.176 100 > 512 2349615.976 9624.027 100 > 1024 1197313.173 9808.39 100 > 1500 822365.834 9868.39 100 > > So the gain for avoiding the hash is marginal and certainly not equal to the gain achieved when enabled with bonding as you reported. > > Even testing with scaling high number of flows there is a similar pattern. > > (Master - No hash when EMC disabled) > Packet Size RX Rate PPs RX Rate Mbps RX % > 64 13418452.15 6870.248 45.086 > > (Patch - hash applied when EMC disabled) > Packet Size RX Rate PPs RX Rate Mbps RX % > 64 12958087.12 6634.541 43.539 > > With this in mind I'll push this to master today. Thanks for testing and applying. What about backporting? The issue affects all branches since 2.9. Applies cleanly down to 2.10. I could send separate patch for 2.9 if needed. > > Ian > >> Best regards, Ilya Maximets. >> >>> lib/dpif-netdev.c | 21 +++++++-------------- >>> 1 file changed, 7 insertions(+), 14 deletions(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index 4d6d0c372..35cd00712 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -6438,20 +6438,13 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, >>> miniflow_extract(packet, &key->mf); >>> key->len = 0; /* Not computed yet. */ >>> - /* If EMC and SMC disabled skip hash computation */ >>> - if (smc_enable_db == true || cur_min != 0) { >>> - 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); >>> - } >>> - } >>> - if (cur_min) { >>> - flow = emc_lookup(&cache->emc_cache, key); >>> - } else { >>> - flow = NULL; >>> - } >>> + key->hash = >>> + (md_is_valid == false) >>> + ? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf) >>> + : dpif_netdev_packet_get_rss_hash(packet, &key->mf); >>> + >>> + /* If EMC is disabled skip emc_lookup */ >>> + flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL; >>> if (OVS_LIKELY(flow)) { >>> tcp_flags = miniflow_get_tcp_flags(&key->mf); >>> n_emc_hit++; >>> > > >
On 4/18/2019 1:43 PM, Ilya Maximets wrote: > > > On 18.04.2019 11:08, Ian Stokes wrote: >> On 4/18/2019 8:55 AM, Ilya Maximets wrote: >>> On 11.03.2019 19:31, Ilya Maximets wrote: >>>> This partially reverts commit bde94613e6276d48a6e0be7a592ebcf9836b4aaf. >>>> >>>> Commit bde94613e627 was aimed to slightly ( < 1%) increase performance >>>> in the case where EMC disabled, but it avoids RSS hash calculation and >>>> OVS has to calculate it while executing OVS_ACTION_ATTR_HASH in order >>>> to handle balanced-tcp bonding. At the time of executing that action >>>> there is no parsed flow, and OVS parses the packet for the second time >>>> to calculate the hash. This happens for all packets received from the >>>> virtual interfaces because they have no HW RSS. >>>> >>>> Here is the example of 'perf' output for VM --> (bonded PHY) traffic: >>>> >>>> Samples: 401K of event 'cycles', Event count (approx.): 50964771478 >>>> Overhead Shared Object Symbol >>>> 27.50% ovs-vswitchd [.] dpcls_lookup.370382 >>>> 16.30% ovs-vswitchd [.] rte_vhost_dequeue_burst.9267 >>>> 14.95% ovs-vswitchd [.] miniflow_extract >>>> 7.22% ovs-vswitchd [.] flow_extract >>>> 7.10% ovs-vswitchd [.] dp_netdev_input__.371002.4826 >>>> 4.01% ovs-vswitchd [.] fast_path_processing.370987.4893 >>>> >>>> We can see that packet parsed twice. First time by 'miniflow_extract' >>>> right after receiving and the second time by 'flow_extract' while >>>> executing actions. >>>> >>>> In this particular case calculating RSS on receive saves > 7% of the >>>> total CPU processing time. It varies from ~7 to ~10 % depending on >>>> scenario/traffic types. >>>> >>>> It's better to calculate hash each time because performance >>>> improvements of avoiding are negligible in compare with performance >>>> drop in case of sending packets to bonded interface. >>>> >>>> Another solution could be to pass the parsed flow explicitly through >>>> the datapath, but this will require big code changes and will have >>>> additional overhead for metadata updating on packet changes. >>>> >>>> Also, this change should have small impact since SMC works well in most >>>> cases and will be enabled/recommended by default in the future. >>>> >>>> CC: Antonio Fischetti <antonio.fischetti@intel.com> >>>> Fixes: bde94613e627 ("dpif-netdev: Avoid reading RSS hash when EMC is disabled.") >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>> --- >>> >>> Hi. >>> Any thoughts about this patch? >>> This is a big performance issue on setups with OVS bonding. >>> >> >> Hi Ilya, >> >> I'm just finishing up performance testing I started yesterday with this patch, I'm inclined to agree with you. >> >> From bench marking with master I see the following with EMC disabled with RFC2544 (0% packet loss). >> >> (Master - No hash when EMC disabled) >> Packet Size RX Rate PPs RX Rate Mbps RX % >> 64 6824334.554 3494.059 45.859 >> 128 6878122.822 7043.198 81.438 >> 256 4528973.86 9275.338 100 >> 512 2349613.015 9624.015 100 >> 1024 1197312.023 9808.38 100 >> 1500 822368.143 9868.418 100 >> >> With the revert applied I see >> >> (Patch - hash applied when EMC disabled) >> Packet Size RX Rate PPs RX Rate Mbps RX % >> 64 6479041.962 3317.269 43.539 >> 128 6747525.93 6909.467 79.891 >> 256 4528894.363 9275.176 100 >> 512 2349615.976 9624.027 100 >> 1024 1197313.173 9808.39 100 >> 1500 822365.834 9868.39 100 >> >> So the gain for avoiding the hash is marginal and certainly not equal to the gain achieved when enabled with bonding as you reported. >> >> Even testing with scaling high number of flows there is a similar pattern. >> >> (Master - No hash when EMC disabled) >> Packet Size RX Rate PPs RX Rate Mbps RX % >> 64 13418452.15 6870.248 45.086 >> >> (Patch - hash applied when EMC disabled) >> Packet Size RX Rate PPs RX Rate Mbps RX % >> 64 12958087.12 6634.541 43.539 >> >> With this in mind I'll push this to master today. > > Thanks for testing and applying. > > What about backporting? The issue affects all branches since 2.9. > Applies cleanly down to 2.10. I could send separate patch for 2.9 > if needed. > Yes, good point, I've pushed 2.11, 2.10. and a reworked patch for 2.9. Thanks Ian
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 4d6d0c372..35cd00712 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -6438,20 +6438,13 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, miniflow_extract(packet, &key->mf); key->len = 0; /* Not computed yet. */ - /* If EMC and SMC disabled skip hash computation */ - if (smc_enable_db == true || cur_min != 0) { - 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); - } - } - if (cur_min) { - flow = emc_lookup(&cache->emc_cache, key); - } else { - flow = NULL; - } + key->hash = + (md_is_valid == false) + ? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf) + : dpif_netdev_packet_get_rss_hash(packet, &key->mf); + + /* If EMC is disabled skip emc_lookup */ + flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL; if (OVS_LIKELY(flow)) { tcp_flags = miniflow_get_tcp_flags(&key->mf); n_emc_hit++;
This partially reverts commit bde94613e6276d48a6e0be7a592ebcf9836b4aaf. Commit bde94613e627 was aimed to slightly ( < 1%) increase performance in the case where EMC disabled, but it avoids RSS hash calculation and OVS has to calculate it while executing OVS_ACTION_ATTR_HASH in order to handle balanced-tcp bonding. At the time of executing that action there is no parsed flow, and OVS parses the packet for the second time to calculate the hash. This happens for all packets received from the virtual interfaces because they have no HW RSS. Here is the example of 'perf' output for VM --> (bonded PHY) traffic: Samples: 401K of event 'cycles', Event count (approx.): 50964771478 Overhead Shared Object Symbol 27.50% ovs-vswitchd [.] dpcls_lookup.370382 16.30% ovs-vswitchd [.] rte_vhost_dequeue_burst.9267 14.95% ovs-vswitchd [.] miniflow_extract 7.22% ovs-vswitchd [.] flow_extract 7.10% ovs-vswitchd [.] dp_netdev_input__.371002.4826 4.01% ovs-vswitchd [.] fast_path_processing.370987.4893 We can see that packet parsed twice. First time by 'miniflow_extract' right after receiving and the second time by 'flow_extract' while executing actions. In this particular case calculating RSS on receive saves > 7% of the total CPU processing time. It varies from ~7 to ~10 % depending on scenario/traffic types. It's better to calculate hash each time because performance improvements of avoiding are negligible in compare with performance drop in case of sending packets to bonded interface. Another solution could be to pass the parsed flow explicitly through the datapath, but this will require big code changes and will have additional overhead for metadata updating on packet changes. Also, this change should have small impact since SMC works well in most cases and will be enabled/recommended by default in the future. CC: Antonio Fischetti <antonio.fischetti@intel.com> Fixes: bde94613e627 ("dpif-netdev: Avoid reading RSS hash when EMC is disabled.") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/dpif-netdev.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)