On 1 Jul 2026, at 15:58, Eli Britstein wrote:
> The started field is currently 'bool', accessed in netdev-dpdk only
> under a mutex lock. In netdev-doca it will be used not under a lock.
> Move it to be atomic as a pre-step towards it.
>
> Signed-off-by: Eli Britstein <[email protected]>
Hi Eli,
Thanks for the updated version; however, I seem to have changed
my mind on the atomics after going over it again in detail.
See below.
//Eelco
> ---
> lib/netdev-dpdk-common.h | 11 ++++++++++-
> lib/netdev-dpdk.c | 12 +++++++-----
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/lib/netdev-dpdk-common.h b/lib/netdev-dpdk-common.h
> index 547c3d4c6..16e5e8767 100644
> --- a/lib/netdev-dpdk-common.h
> +++ b/lib/netdev-dpdk-common.h
> @@ -118,7 +118,7 @@ struct netdev_dpdk_common {
> uint16_t port_id;
> bool attached;
> bool is_representor;
> - bool started;
> + atomic_bool started;
> struct eth_addr hwaddr;
> int mtu;
> int socket_id;
> @@ -180,4 +180,13 @@ netdev_dpdk_common_cast(const struct netdev *netdev)
> return CONTAINER_OF(netdev, struct netdev_dpdk_common, up);
> }
>
> +static inline bool
> +dpdk_dev_is_started(struct netdev_dpdk_common *common)
In v3 you committed to change this to netdev_dpdk_is_started().
> +{
> + bool started;
> +
> + atomic_read_relaxed(&common->started, &started);
> + return started;
> +}
> +
> #endif /* NETDEV_DPDK_COMMON_H */
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 667be4f23..78eaa8706 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1283,7 +1283,6 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> rte_strerror(-diag));
> return -diag;
> }
> - dev->common.started = true;
>
> netdev_dpdk_configure_xstats(&dev->common);
>
> @@ -1303,6 +1302,9 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>
> mbp_priv = rte_mempool_get_priv(dev->common.dpdk_mp->mp);
> dev->buf_size = mbp_priv->mbuf_data_room_size - RTE_PKTMBUF_HEADROOM;
> +
> + atomic_store_explicit(&dev->common.started, true, memory_order_seq_cst);
I went over the atomic use case again, and I have some questions,
suggestions:
The writes here and in the doca patch use seq_cst, while the
read in dpdk_dev_is_started() uses relaxed. In netdev-dpdk
this is fine because the mutex provides the necessary ordering.
However, in netdev-doca, dpdk_dev_is_started() is called
without a lock; a relaxed read will not pair with the release
implied by seq_cst, so the reader could see started==true
before prior initialization is visible. This should be a
release/acquire pair. Change the stores to
memory_order_release and the read to memory_order_acquire:
atomic_store_explicit(&dev->common.started, true,
memory_order_release);
atomic_read_explicit(&common->started, &started,
memory_order_acquire);
So for DPDK it's fine due to the lock, so we should either add
some comment about it, or also update the stores in this patch
to use memory_order_release for consistency with the doca side.
> return 0;
> }
>
> @@ -1369,7 +1371,7 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no, int socket_id)
> dev->vhost_reconfigured = false;
> dev->virtio_features_state = OVS_VIRTIO_F_CLEAN;
> dev->common.attached = false;
> - dev->common.started = false;
> + atomic_init(&dev->common.started, false);
>
> ovsrcu_init(&dev->qos_conf, NULL);
>
> @@ -1578,7 +1580,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
> dpdk_rx_steer_unconfigure(dev);
>
> rte_eth_dev_stop(dev->common.port_id);
> - dev->common.started = false;
> + atomic_store_explicit(&dev->common.started, false, memory_order_seq_cst);
>
> if (dev->common.attached) {
> bool dpdk_resources_still_used = false;
> @@ -6088,7 +6090,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
> && dev->common.txq_size == dev->common.requested_txq_size
> && eth_addr_equals(dev->common.hwaddr, dev->common.requested_hwaddr)
> && dev->common.socket_id == dev->common.requested_socket_id
> - && dev->common.started && !pending_reset) {
> + && dpdk_dev_is_started(&dev->common) && !pending_reset) {
> /* Reconfiguration is unnecessary */
>
> goto out;
> @@ -6110,7 +6112,7 @@ retry:
> rte_eth_dev_stop(dev->common.port_id);
> }
>
> - dev->common.started = false;
> + atomic_store_explicit(&dev->common.started, false, memory_order_seq_cst);
>
> err = netdev_dpdk_mempool_configure(dev);
> if (err && err != EEXIST) {
> --
> 2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
