git sha: 1fce8af49ca4563047827aead37339a4891523b7
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload: Move hw_miss_packet_postprocess netdev API to dpif.
This patch moves the hardware packet miss recovery API from the
netdev-offload to the dpif-offload provider. The API name has been
changed from hw_miss_packet_recover() to hw_miss_packet_postprocess()
to reflect that it may also be used for other tasks in the future,
such as conntrack post-processing.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 82df85823..5d32a47b3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8423,14 +8423,18 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread
> *pmd,
> #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */
> /* Restore the packet if HW processing was terminated before completion.
> */
> struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq;
> - bool miss_api_supported;
> + bool postprocess_api_supported;
> +
> +
> atomic_read_relaxed(&rxq->port->netdev->hw_info.postprocess_api_supported,
> + &postprocess_api_supported);
> + if (postprocess_api_supported) {
> + int err = dpif_offload_netdev_hw_miss_packet_postprocess(
> + rxq->port->netdev, packet);
>
> - atomic_read_relaxed(&rxq->port->netdev->hw_info.miss_api_supported,
> - &miss_api_supported);
> - if (miss_api_supported) {
> - int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet);
> if (err && err != EOPNOTSUPP) {
> - COVERAGE_INC(datapath_drop_hw_miss_recover);
> + if (err != ECANCELED) {
> + COVERAGE_INC(datapath_drop_hw_miss_postprocess);
> + }
> return -1;
> }
> }
This introduces a behavior change in error handling. The original code
incremented coverage for all errors except EOPNOTSUPP, but now the code
only increments coverage if err != ECANCELED. Does this change in
coverage reporting mask potentially important error conditions?
[ ... ]
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index bb2a63323..12dac4239 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -835,6 +835,42 @@ dpif_offload_netdev_flush_flows(struct netdev *netdev)
> return EOPNOTSUPP;
> }
>
> +int
> +dpif_offload_netdev_hw_miss_packet_postprocess(struct netdev *netdev,
> + struct dp_packet *packet)
> +{
> + const struct dpif_offload *offload;
> + bool postprocess_api_supported;
> + int rc;
> +
> + atomic_read_relaxed(&netdev->hw_info.postprocess_api_supported,
> + &postprocess_api_supported);
> + if (!postprocess_api_supported) {
> + return EOPNOTSUPP;
> + }
> +
> + offload = ovsrcu_get(const struct dpif_offload *, &netdev->dpif_offload);
> +
> + if (!offload || !offload->class->netdev_hw_miss_packet_postprocess) {
> + if (offload) {
> + /* Offload is configured and API unsupported by the port;
> + * avoid subsequent calls. */
> + atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
> + false);
> + }
> + return EOPNOTSUPP;
> + }
> +
> + rc = offload->class->netdev_hw_miss_packet_postprocess(offload, netdev,
> + packet);
> + if (rc == EOPNOTSUPP) {
> + /* API unsupported by the port; avoid subsequent calls. */
> + atomic_store_relaxed(&netdev->hw_info.postprocess_api_supported,
> + false);
> + }
> + return rc;
> +}
> +
Could this function race with initialization where postprocess_api_supported
gets set to false after being initially set to true? What happens if the
atomic flag gets modified between checking it and using the offload pointer?
[ ... ]
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 0c4209290..be5787f4d 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -186,7 +186,8 @@ 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);
> - atomic_store_relaxed(&netdev->hw_info.miss_api_supported, true);
> + atomic_store_relaxed(&netdev->hw_info.postprocess_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);
This unconditionally sets postprocess_api_supported to true when any flow
API initializes successfully. However, not all flow APIs may support the
postprocess functionality. Should this check if the specific API actually
supports postprocessing before setting the flag?
[ ... ]
> -int
> -netdev_hw_miss_packet_recover(struct netdev *netdev,
> - struct dp_packet *packet)
> -{
> - const struct netdev_flow_api *flow_api;
> - bool miss_api_supported;
> - int rv;
> -
> - atomic_read_relaxed(&netdev->hw_info.miss_api_supported,
> - &miss_api_supported);
> - if (!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. */
> - atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false);
> - }
> -
> - return rv;
> -}
The removed function netdev_hw_miss_packet_recover had different error
handling behavior compared to the new
dpif_offload_netdev_hw_miss_packet_postprocess.
The old version only disabled the flag on EOPNOTSUPP, while the new version
in dpif-netdev.c has special handling for ECANCELED. Could this behavioral
change cause issues for existing callers?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
