| Message ID | 20210723211123.3566655-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpif-netdev: Fix non-atomic read of smc_enable_db. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
On 23 Jul 2021, at 23:11, Ilya Maximets wrote: > Atomic variables must be read atomically. And since we already have > 'smc_enable_db' in the PMD context, we need to use it from there to > avoid reading atomically twice. > > Also, 'smc_enable_db' is a global configuration, there is no need > to read it per-port or per-rxq. > > CC: Harry van Haaren <harry.van.haaren@intel.com> > Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Changes look good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Atomic variables must be read atomically. And since we already have > 'smc_enable_db' in the PMD context, we need to use it from there to > avoid reading atomically twice. > > Also, 'smc_enable_db' is a global configuration, there is no need > to read it per-port or per-rxq. > > CC: Harry van Haaren <harry.van.haaren@intel.com> > Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- This LGTM too. Agreed that it doesn't need to be read per-port or per-RXQ. Acked-by: Cian Ferriter <cian.ferriter@intel.com> I also tested this by running a scenario with multiple PMD threads and toggling the SMC on and off with EMC off always: $OVS_DIR/utilities/ovs-vsctl --no-wait set Open_vSwitch . other_config:smc-enable=true $OVS_DIR/utilities/ovs-vsctl --no-wait set Open_vSwitch . other_config:smc-enable=false I checked pmd-perf-show to see that SMC was being enabled and disabled correctly. It was. watch -d -n 1 $OVS_DIR/utilities/ovs-appctl dpif-netdev/pmd-perf-show
On 7/27/21 4:16 PM, Ferriter, Cian wrote: > > >> Atomic variables must be read atomically. And since we already have >> 'smc_enable_db' in the PMD context, we need to use it from there to >> avoid reading atomically twice. >> >> Also, 'smc_enable_db' is a global configuration, there is no need >> to read it per-port or per-rxq. >> >> CC: Harry van Haaren <harry.van.haaren@intel.com> >> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.") >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > This LGTM too. > Agreed that it doesn't need to be read per-port or per-RXQ. > Acked-by: Cian Ferriter <cian.ferriter@intel.com> > > I also tested this by running a scenario with multiple PMD threads and toggling the SMC on and off with EMC off always: > $OVS_DIR/utilities/ovs-vsctl --no-wait set Open_vSwitch . other_config:smc-enable=true > $OVS_DIR/utilities/ovs-vsctl --no-wait set Open_vSwitch . other_config:smc-enable=false > > I checked pmd-perf-show to see that SMC was being enabled and disabled correctly. It was. > watch -d -n 1 $OVS_DIR/utilities/ovs-appctl dpif-netdev/pmd-perf-show > Thanks, Eelco and Cian. Applied to master and branch-2.16. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 59e326f11..d97c5756f 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3078,11 +3078,9 @@ smc_insert(struct dp_netdev_pmd_thread *pmd, struct smc_bucket *bucket = &smc_cache->buckets[key->hash & SMC_MASK]; uint16_t index; uint32_t cmap_index; - bool smc_enable_db; int i; - atomic_read_relaxed(&pmd->dp->smc_enable_db, &smc_enable_db); - if (!smc_enable_db) { + if (!pmd->ctx.smc_enable_db) { return; } @@ -5915,6 +5913,9 @@ dpif_netdev_run(struct dpif *dpif) non_pmd = dp_netdev_get_pmd(dp, NON_PMD_CORE_ID); if (non_pmd) { ovs_mutex_lock(&dp->non_pmd_mutex); + + atomic_read_relaxed(&dp->smc_enable_db, &non_pmd->ctx.smc_enable_db); + HMAP_FOR_EACH (port, node, &dp->ports) { if (!netdev_is_pmd(port->netdev)) { int i; @@ -5926,8 +5927,6 @@ dpif_netdev_run(struct dpif *dpif) non_pmd->ctx.emc_insert_min = 0; } - non_pmd->ctx.smc_enable_db = dp->smc_enable_db; - for (i = 0; i < port->n_rxq; i++) { if (!netdev_rxq_enabled(port->rxqs[i].rx)) { @@ -6190,6 +6189,8 @@ reload: pmd_perf_start_iteration(s); + atomic_read_relaxed(&pmd->dp->smc_enable_db, &pmd->ctx.smc_enable_db); + for (i = 0; i < poll_cnt; i++) { if (!poll_list[i].rxq_enabled) { @@ -6203,8 +6204,6 @@ reload: pmd->ctx.emc_insert_min = 0; } - pmd->ctx.smc_enable_db = pmd->dp->smc_enable_db; - process_packets = dp_netdev_process_rxq_port(pmd, poll_list[i].rxq, poll_list[i].port_no); @@ -7328,11 +7327,9 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, const bool netdev_flow_api = netdev_is_flow_api_enabled(); int i; uint16_t tcp_flags; - bool smc_enable_db; size_t map_cnt = 0; bool batch_enable = true; - atomic_read_relaxed(&pmd->dp->smc_enable_db, &smc_enable_db); pmd_perf_update_counter(&pmd->perf_stats, md_is_valid ? PMD_STAT_RECIRC : PMD_STAT_RECV, cnt); @@ -7434,7 +7431,7 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, n_mfex_opt_hit); pmd_perf_update_counter(&pmd->perf_stats, PMD_STAT_EXACT_HIT, n_emc_hit); - if (!smc_enable_db) { + if (!pmd->ctx.smc_enable_db) { return dp_packet_batch_size(packets_); }
Atomic variables must be read atomically. And since we already have 'smc_enable_db' in the PMD context, we need to use it from there to avoid reading atomically twice. Also, 'smc_enable_db' is a global configuration, there is no need to read it per-port or per-rxq. CC: Harry van Haaren <harry.van.haaren@intel.com> Fixes: 9ac84a1a3698 ("dpif-avx512: Add ISA implementation of dpif.") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/dpif-netdev.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)