Re: [dpdk-dev] [PATCH v2 1/2] eal: add API that sleeps while waiting for threads

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

HI Ferruh,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Monday, October 15, 2018 11:21 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; stephen@networkplumber.org
> Subject: [dpdk-dev] [PATCH v2 1/2] eal: add API that sleeps while waiting for threads
> 
> It is common that sample applications call rte_eal_wait_lcore() while
> waiting for worker threads to be terminated.
> Mostly master lcore keeps waiting in this function.
> 
> The waiting app for termination is not a time critical task, app can
> prefer a sleep version of the waiting to consume less cycles.
> 
> A sleeping version of the API, rte_eal_wait_lcore_sleep(), has been
> added which uses pthread conditions.
> 
> Sample applications will be updated later to use this API.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> v2:
> * use pthread cond instead of usleep
> ---
>  lib/librte_eal/bsdapp/eal/eal.c            |  3 +++
>  lib/librte_eal/bsdapp/eal/eal_thread.c     |  7 ++++++
>  lib/librte_eal/common/eal_common_launch.c  | 22 ++++++++++++++++++
>  lib/librte_eal/common/include/rte_launch.h | 26 ++++++++++++++++++++++
>  lib/librte_eal/common/include/rte_lcore.h  |  3 +++
>  lib/librte_eal/linuxapp/eal/eal.c          |  3 +++
>  lib/librte_eal/linuxapp/eal/eal_thread.c   |  7 ++++++
>  lib/librte_eal/rte_eal_version.map         |  1 +
>  8 files changed, 72 insertions(+)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 7735194a3..e7d676657 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -756,6 +756,9 @@ rte_eal_init(int argc, char **argv)
>  		snprintf(thread_name, sizeof(thread_name),
>  				"lcore-slave-%d", i);
>  		rte_thread_setname(lcore_config[i].thread_id, thread_name);
> +
> +		pthread_mutex_init(&rte_eal_thread_mutex[i], NULL);
> +		pthread_cond_init(&rte_eal_thread_cond[i], NULL);
>  	}
> 
>  	/*
> diff --git a/lib/librte_eal/bsdapp/eal/eal_thread.c b/lib/librte_eal/bsdapp/eal/eal_thread.c
> index 309b58726..60db32d57 100644
> --- a/lib/librte_eal/bsdapp/eal/eal_thread.c
> +++ b/lib/librte_eal/bsdapp/eal/eal_thread.c
> @@ -28,6 +28,9 @@ RTE_DEFINE_PER_LCORE(unsigned, _lcore_id) = LCORE_ID_ANY;
>  RTE_DEFINE_PER_LCORE(unsigned, _socket_id) = (unsigned)SOCKET_ID_ANY;
>  RTE_DEFINE_PER_LCORE(rte_cpuset_t, _cpuset);
> 
> +pthread_cond_t rte_eal_thread_cond[RTE_MAX_LCORE];
> +pthread_mutex_t rte_eal_thread_mutex[RTE_MAX_LCORE];

I think would be better to include cond and mutex into struct lcore_config itself,
probably would help to avoid false sharing.
Though yeh, it would mean ABI breakage, I suppose. 

> +
>  /*
>   * Send a message to a slave lcore identified by slave_id to call a
>   * function f with argument arg. Once the execution is done, the
> @@ -154,6 +157,10 @@ eal_thread_loop(__attribute__((unused)) void *arg)
>  		lcore_config[lcore_id].ret = ret;
>  		rte_wmb();
>  		lcore_config[lcore_id].state = FINISHED;
> +
> +		pthread_mutex_lock(&rte_eal_thread_mutex[lcore_id]);
> +		pthread_cond_signal(&rte_eal_thread_cond[lcore_id]);
> +		pthread_mutex_unlock(&rte_eal_thread_mutex[lcore_id]);

I understand it would work that way too, but if you introduce mutex and cond around  
the state, then it is better to manipulate/access the state after grabbing the mutex.
BTW in that case we don't need wmb:

lcore_config[lcore_id].ret = ret;
pthread_mutex_lock(...);
lcore_config[lcore_id].state = FINISHED;
pthread_cond_signal(..);
pthread_mutex_unlock(...);

Konstantin