|SUCCESS| [6/6] net/hinic: fix mutexes for multi-process

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

6af59b8914f6e1a3d7e6bb1f5f6f4b8f5e6e6f1e
Author: Stephen Hemminger <stephen at networkplumber.org>
ethdev: fix flow_ops_mutex for multi-process

This series fixes missing PTHREAD_PROCESS_SHARED attributes on mutexes
in shared memory across multiple DPDK drivers.

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index 2ec665be0f..d12395efe0 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -89,6 +89,22 @@ eth_dev_set_dummy_fops(struct rte_eth_dev *eth_dev)
>  	eth_dev->recycle_rx_descriptors_refill = rte_eth_recycle_rx_descriptors_refill_dummy;
>  }
>  
> +/*
> + * Initialize mutex for process-shared access.
> + * The rte_eth_dev_data structure is in shared memory, so any mutex
> + * protecting it must use PTHREAD_PROCESS_SHARED.
> + */
> +static void
> +eth_dev_flow_ops_mutex_init(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() and
pthread_mutexattr_setpshared()? Can eth_dev_flow_ops_mutex_init() fail
if these calls fail?

> @@ -135,7 +151,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);
> +	eth_dev_flow_ops_mutex_init(&eth_dev->data->flow_ops_mutex);

Does rte_eth_dev_allocate() need to handle potential failures from
eth_dev_flow_ops_mutex_init()?

> 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 this code check the return value from pthread_mutex_init()?

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

Does this code leak the mutex if pthread_mutexattr_setpshared() fails
after pthread_mutex_init() has already been called in an earlier code
path?

[ ... ]

> diff --git a/drivers/net/bnxt/bnxt_util.c b/drivers/net/bnxt/bnxt_util.c
> index aa184496c2..c3f0a03f25 100644
> --- a/drivers/net/bnxt/bnxt_util.c
> +++ b/drivers/net/bnxt/bnxt_util.c
> @@ -3,6 +3,7 @@
>   * All rights reserved.
>   */
>  
> +#include <pthread.h>
>  #include <inttypes.h>
>  #include <rte_ether.h>
>  
> @@ -37,3 +38,15 @@ uint8_t hweight32(uint32_t word32)
>  	res = res + (res >> 8);
>  	return (res + (res >> 16)) & 0x000000FF;
>  }
> +
> +/* Initialize mutex so that it can be shared between processes */
> +void
> +bnxt_init_mutex(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 bnxt_init_mutex() check return values from
pthread_mutexattr_init(), pthread_mutexattr_setpshared(), and
pthread_mutex_init()?

> diff --git a/drivers/net/bnxt/bnxt_txq.c b/drivers/net/bnxt/bnxt_txq.c
> index 7752f06eb7..8c834acb1d 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);
> +	bnxt_init_mutex(&txq->txq_lock);
> +	return 0;

Does bnxt_tx_queue_setup_op() handle potential failures from
bnxt_init_mutex()?

[ ... ]

> diff --git a/drivers/net/hinic/base/hinic_compat.h b/drivers/net/hinic/base/hinic_compat.h
> index d3994c50e9..18148a119e 100644
> --- a/drivers/net/hinic/base/hinic_compat.h
> +++ b/drivers/net/hinic/base/hinic_compat.h
> @@ -197,10 +197,21 @@ static inline u16 ilog2(u32 n)
>  	return res;
>  }
>  
> +/*
> + * Initialize mutex for process-shared access.
> + * Structures may be in shared memory accessible by multiple processes,
> + * so mutexes must use PTHREAD_PROCESS_SHARED.
> + */
>  static inline int hinic_mutex_init(pthread_mutex_t *pthreadmutex,
>  					const pthread_mutexattr_t *mattr)
>  {
> -	return pthread_mutex_init(pthreadmutex, mattr);
> +	pthread_mutexattr_t attr;
> +
> +	pthread_mutexattr_init(&attr);
> +	pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_mutex_init(pthreadmutex, mattr ? mattr : &attr);
> +	pthread_mutexattr_destroy(&attr);
> +	return 0;

Does this code always return 0 even if pthread_mutexattr_init(),
pthread_mutexattr_setpshared(), or pthread_mutex_init() fail?