|SUCCESS| [v3,7/7] net/hinic: fix mutexes for multi-process

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163451

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-30

80d1a9aff7f6 ("ethdev: make flow API thread safe")
Author: Stephen Hemminger <stephen at networkplumber.org>

eal: add helper to initialize process-shared mutex

This series adds rte_thread_mutex_init_shared() to correctly initialize
mutexes in shared memory for use across DPDK processes, and applies it
to multiple drivers.

> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 8da9d4d3fb..27d5b9c594 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2022 Microsoft Corporation
>   */
>  
> +#include <pthread.h>
>  #include <stdint.h>
>  
>  #include <rte_os.h>

Does including pthread.h here expose POSIX types in a public header that
might conflict on non-POSIX systems? Should this be conditional or in the
implementation files only?

> +/**
> + * Initialize a pthread mutex for use in shared memory accessible by
> + * multiple DPDK processes.
> + *
> + * Sets PTHREAD_PROCESS_SHARED so the mutex can synchronize threads
> + * across DPDK primary and secondary processes when the mutex resides
> + * in shared memory (e.g. hugepage memory).
> + *
> + * @param mutex
> + *   The mutex to initialize.
> + */
> +__rte_internal
> +void rte_thread_mutex_init_shared(pthread_mutex_t *mutex);

Does this function need to return void, or should it return int to report
pthread_mutex_init() failures to the caller?

> diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c
> index 950c0848ba..f3a6bd2ad3 100644
> --- a/lib/eal/unix/rte_thread.c
> +++ b/lib/eal/unix/rte_thread.c
> @@ -419,3 +419,15 @@ rte_thread_get_affinity_by_id(rte_thread_t thread_id,
>  	return pthread_getaffinity_np((pthread_t)thread_id.opaque_id,
>  		sizeof(*cpuset), cpuset);
>  }
> +
> +RTE_EXPORT_INTERNAL_SYMBOL(rte_thread_mutex_init_shared)
> +void
> +rte_thread_mutex_init_shared(pthread_mutex_t *mutex)
> +{
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(mutex, &attr);
> +	pthread_mutexattr_destroy(&attr);
> +}

Does this code check return values from pthread_mutexattr_init(),
pthread_mutexattr_setpshared(), or pthread_mutex_init()? Should failures
be logged or handled?

[ ... ]

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index 2ec665be0f..70ddce5bfc 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -135,7 +135,7 @@ rte_eth_dev_allocate(const char *name)
>  	eth_dev->data->port_id = port_id;
>  	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
>  	eth_dev->data->mtu = RTE_ETHER_MTU;
> -	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
> +	rte_thread_mutex_init_shared(&eth_dev->data->flow_ops_mutex);
>  	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
>  	eth_dev_shared_data->allocated_ports++;

Should this check that rte_thread_mutex_init_shared() succeeded before
continuing, or does failure leave flow_ops_mutex in an undefined state?

[ ... ]

> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index 3e590d38f7..aa3fe53b22 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -144,11 +144,20 @@ fs_mutex_init(struct fs_priv *priv)
>  	/* Allow mutex relocks for the thread holding the mutex. */
>  	ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
>  	if (ret) {
> -		ERROR("Cannot set mutex type - %s", strerror(ret));
> -		return ret;
> +		ERROR("Cannot set mutex recursive - %s", strerror(ret));
> +		goto out;
> +	}
> +	/* Allow mutex to be shared between processes. */
> +	ret = pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	if (ret) {
> +		ERROR("Cannot set mutex pshared - %s", strerror(ret));
> +		goto out;
>  	}
> +	pthread_mutex_init(&priv->hotplug_mutex, &attr);

Does the return value of pthread_mutex_init() need checking here?

> -	return pthread_mutex_init(&priv->hotplug_mutex, &attr);
> +out:
> +	pthread_mutexattr_destroy(&attr);
> +	return ret;
>  }

[ ... ]

> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index cfcd880961..9491f18b7c 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -8,10 +8,10 @@
>  #include "axgbe_common.h"
>  #include "axgbe_phy.h"
>  #include "axgbe_regs.h"
> -#include "rte_time.h"
> -
> -#include "eal_filesystem.h"
>  
> +#include <rte_thread.h>
> +#include <rte_time.h>
> +#include <eal_filesystem.h>
>  #include <rte_vect.h>

Does including eal_filesystem.h without angle brackets work consistently,
or should it remain quoted?

[ ... ]

> diff --git a/drivers/net/bnxt/bnxt_txq.c b/drivers/net/bnxt/bnxt_txq.c
> index 7752f06eb7..03407c556a 100644
> --- a/drivers/net/bnxt/bnxt_txq.c
> +++ b/drivers/net/bnxt/bnxt_txq.c
> @@ -204,7 +204,8 @@ int bnxt_tx_queue_setup_op(struct rte_eth_dev *eth_dev,
>  		goto err;
>  	}
>  
> -	return pthread_mutex_init(&txq->txq_lock, NULL);
> +	rte_thread_mutex_init_shared(&txq->txq_lock);
> +	return 0;

Does ignoring any potential failure from rte_thread_mutex_init_shared()
leave txq_lock in an unusable state for later operations?

[ ... ]