| Message ID | 20220630085342.3483802-1-elibr@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-offload: Set 'miss_api_supported' to be under netdev | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 6/30/22 10:53, Eli Britstein wrote: > Commit [1] introduced a flag in dpif-netdev level, to optimize > performance and avoid hw_miss_packet_recover() for devices with no such > support. > However, there is a race condition between traffic processing and > assigning a 'flow_api' object to the netdev. In such case, EOPNOTSUPP is > returned by netdev_hw_miss_packet_recover() in netdev-offload.c layer > because 'flow_api' is not yet initialized. As a result, the flag is > falsely disabled, and subsequent packets won't be recovered, though they > should. > > In order to fix it, move the flag to be in netdev-offload layer, to > avoid that race. > > [1]: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") > > Fixes: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") > Signed-off-by: Eli Britstein <elibr@nvidia.com> > --- Hi, Eli. Sorry for the late reply. Thanks for the fix! The change looks fine to me in general, see some comments inline. Best regards, Ilya Maximets. > lib/dpif-netdev.c | 15 ++++----------- > lib/netdev-offload.c | 25 ++++++++++++++++++++----- > lib/netdev-offload.h | 1 + > 3 files changed, 25 insertions(+), 16 deletions(-) > > Github actions: > - v1: https://github.com/elibritstein/OVS/actions/runs/2587743230 > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index f46b9fe183..a286050b57 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -431,7 +431,6 @@ struct dp_netdev_rxq { > unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ > struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ > bool is_vhost; /* Is rxq of a vhost port. */ > - bool hw_miss_api_supported; /* hw_miss_packet_recover() supported.*/ > > /* Counters of cycles spent successfully polling and processing pkts. */ > atomic_ullong cycles[RXQ_N_CYCLES]; > @@ -5421,7 +5420,6 @@ port_reconfigure(struct dp_netdev_port *port) > > port->rxqs[i].port = port; > port->rxqs[i].is_vhost = !strncmp(port->type, "dpdkvhost", 9); > - port->rxqs[i].hw_miss_api_supported = true; > > err = netdev_rxq_open(netdev, &port->rxqs[i].rx, i); > if (err) { > @@ -8039,16 +8037,11 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd, > /* Restore the packet if HW processing was terminated before completion. */ > struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq; > > - if (rxq->hw_miss_api_supported) { > + if (rxq->port->netdev->hw_info.miss_api_supported) { > int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); > - if (err) { > - if (err != EOPNOTSUPP) { > - COVERAGE_INC(datapath_drop_hw_miss_recover); > - return -1; > - } else { > - /* API unsupported by the port; avoid subsequent calls. */ > - rxq->hw_miss_api_supported = false; > - } > + if (err && err != EOPNOTSUPP) { > + COVERAGE_INC(datapath_drop_hw_miss_recover); > + return -1; > } > } > #endif > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c > index fb108c0d50..50c5076a63 100644 > --- a/lib/netdev-offload.c > +++ b/lib/netdev-offload.c > @@ -182,6 +182,7 @@ netdev_assign_flow_api(struct netdev *netdev) > CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { > if (!rfa->flow_api->init_flow_api(netdev)) { > ovs_refcount_ref(&rfa->refcnt); > + netdev->hw_info.miss_api_supported = true; > ovsrcu_set(&netdev->flow_api, rfa->flow_api); > VLOG_INFO("%s: Assigned flow API '%s'.", > netdev_get_name(netdev), rfa->flow_api->type); > @@ -190,6 +191,7 @@ netdev_assign_flow_api(struct netdev *netdev) > VLOG_DBG("%s: flow API '%s' is not suitable.", > netdev_get_name(netdev), rfa->flow_api->type); > } > + netdev->hw_info.miss_api_supported = false; I'm not sure these initializations belong to this function. We should move them to one of the initialization functions instead. Since 'miss_api_supported' flag can in theory be used before the flow api is initialized, it should be correctly initialized in the netdev_open() function. Not sure about the default value. 'false', I guess, should be fine. And then netdev_init_flow_api() can flip it to 'true' once the flow API is initialized. The opposite is also fine. The field is supposed to represent hardware capabilities, so the default value doesn't mean much. > VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); > > return -1; > @@ -263,12 +265,25 @@ int > netdev_hw_miss_packet_recover(struct netdev *netdev, > struct dp_packet *packet) > { > - const struct netdev_flow_api *flow_api = > - ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > + const struct netdev_flow_api *flow_api; > + int rv; > + > + if (!netdev->hw_info.miss_api_supported) { > + return EOPNOTSUPP; > + } > + > + flow_api = ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); > + if (!flow_api || !flow_api->hw_miss_packet_recover) { > + return EOPNOTSUPP; > + } > + > + rv = flow_api->hw_miss_packet_recover(netdev, packet); > + if (rv == EOPNOTSUPP) { > + /* API unsupported by the port; avoid subsequent calls. */ > + netdev->hw_info.miss_api_supported = false; > + } > > - return (flow_api && flow_api->hw_miss_packet_recover) > - ? flow_api->hw_miss_packet_recover(netdev, packet) > - : EOPNOTSUPP; > + return rv; > } > > int > diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h > index 8237a85ddb..c56f9d49ad 100644 > --- a/lib/netdev-offload.h > +++ b/lib/netdev-offload.h > @@ -45,6 +45,7 @@ struct ovs_action_push_tnl; > /* Offload-capable (HW) netdev information */ > struct netdev_hw_info { > bool oor; /* Out of Offload Resources ? */ > + bool miss_api_supported; /* hw_miss_packet_recover() supported.*/ I think, this should be an atomic_bool. It was OK to have a non-atomic read/write access to rxq, since rxq is only used by a single thread at a time. But this value is shared among multiple threads working on the same netdev, so it should be atomic. This should not cause any performance issues, AFAIU, since relaxed atomic read/write on bool should be equal to a normal read/write on most modern architectures, but having it atomic is semantically more correct. > int offload_count; /* Pending (non-offloaded) flow count */ > int pending_count; /* Offloaded flow count */ > OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f46b9fe183..a286050b57 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -431,7 +431,6 @@ struct dp_netdev_rxq { unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ bool is_vhost; /* Is rxq of a vhost port. */ - bool hw_miss_api_supported; /* hw_miss_packet_recover() supported.*/ /* Counters of cycles spent successfully polling and processing pkts. */ atomic_ullong cycles[RXQ_N_CYCLES]; @@ -5421,7 +5420,6 @@ port_reconfigure(struct dp_netdev_port *port) port->rxqs[i].port = port; port->rxqs[i].is_vhost = !strncmp(port->type, "dpdkvhost", 9); - port->rxqs[i].hw_miss_api_supported = true; err = netdev_rxq_open(netdev, &port->rxqs[i].rx, i); if (err) { @@ -8039,16 +8037,11 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd, /* Restore the packet if HW processing was terminated before completion. */ struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq; - if (rxq->hw_miss_api_supported) { + if (rxq->port->netdev->hw_info.miss_api_supported) { int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); - if (err) { - if (err != EOPNOTSUPP) { - COVERAGE_INC(datapath_drop_hw_miss_recover); - return -1; - } else { - /* API unsupported by the port; avoid subsequent calls. */ - rxq->hw_miss_api_supported = false; - } + if (err && err != EOPNOTSUPP) { + COVERAGE_INC(datapath_drop_hw_miss_recover); + return -1; } } #endif diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index fb108c0d50..50c5076a63 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -182,6 +182,7 @@ netdev_assign_flow_api(struct netdev *netdev) CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { if (!rfa->flow_api->init_flow_api(netdev)) { ovs_refcount_ref(&rfa->refcnt); + netdev->hw_info.miss_api_supported = true; ovsrcu_set(&netdev->flow_api, rfa->flow_api); VLOG_INFO("%s: Assigned flow API '%s'.", netdev_get_name(netdev), rfa->flow_api->type); @@ -190,6 +191,7 @@ netdev_assign_flow_api(struct netdev *netdev) VLOG_DBG("%s: flow API '%s' is not suitable.", netdev_get_name(netdev), rfa->flow_api->type); } + netdev->hw_info.miss_api_supported = false; VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); return -1; @@ -263,12 +265,25 @@ int netdev_hw_miss_packet_recover(struct netdev *netdev, struct dp_packet *packet) { - const struct netdev_flow_api *flow_api = - ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + const struct netdev_flow_api *flow_api; + int rv; + + if (!netdev->hw_info.miss_api_supported) { + return EOPNOTSUPP; + } + + flow_api = ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + if (!flow_api || !flow_api->hw_miss_packet_recover) { + return EOPNOTSUPP; + } + + rv = flow_api->hw_miss_packet_recover(netdev, packet); + if (rv == EOPNOTSUPP) { + /* API unsupported by the port; avoid subsequent calls. */ + netdev->hw_info.miss_api_supported = false; + } - return (flow_api && flow_api->hw_miss_packet_recover) - ? flow_api->hw_miss_packet_recover(netdev, packet) - : EOPNOTSUPP; + return rv; } int diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 8237a85ddb..c56f9d49ad 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -45,6 +45,7 @@ struct ovs_action_push_tnl; /* Offload-capable (HW) netdev information */ struct netdev_hw_info { bool oor; /* Out of Offload Resources ? */ + bool miss_api_supported; /* hw_miss_packet_recover() supported.*/ int offload_count; /* Pending (non-offloaded) flow count */ int pending_count; /* Offloaded flow count */ OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */
Commit [1] introduced a flag in dpif-netdev level, to optimize performance and avoid hw_miss_packet_recover() for devices with no such support. However, there is a race condition between traffic processing and assigning a 'flow_api' object to the netdev. In such case, EOPNOTSUPP is returned by netdev_hw_miss_packet_recover() in netdev-offload.c layer because 'flow_api' is not yet initialized. As a result, the flag is falsely disabled, and subsequent packets won't be recovered, though they should. In order to fix it, move the flag to be in netdev-offload layer, to avoid that race. [1]: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") Fixes: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") Signed-off-by: Eli Britstein <elibr@nvidia.com> --- lib/dpif-netdev.c | 15 ++++----------- lib/netdev-offload.c | 25 ++++++++++++++++++++----- lib/netdev-offload.h | 1 + 3 files changed, 25 insertions(+), 16 deletions(-) Github actions: - v1: https://github.com/elibritstein/OVS/actions/runs/2587743230