Re: [PATCH v2 1/3] eal: add rte control thread create API

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2022-12-07 17:38, Tyler Retzlaff wrote:
> On Wed, Dec 07, 2022 at 10:13:39AM +0100, Mattias Rönnblom wrote:
>> On 2022-12-06 18:28, Tyler Retzlaff wrote:
>>> Add rte_control_thread_create API as a replacement for
>>> rte_ctrl_thread_create to allow deprecation of the use of platform
>>> specific types in DPDK public API.
>>>
>>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>> ---
>>>   lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>>>   lib/eal/include/rte_thread.h       | 29 ++++++++++++
>>>   lib/eal/version.map                |  3 ++
>>>   3 files changed, 117 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
>>> index c5d8b43..7950b50 100644
>>> --- a/lib/eal/common/eal_common_thread.c
>>> +++ b/lib/eal/common/eal_common_thread.c
>>> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>>>   };
>>>   struct rte_thread_ctrl_params {
>>> -	void *(*start_routine)(void *);
>>> +	union {
>>> +		void *(*start_routine)(void *);
>>> +		rte_thread_func thread_func;
>>
>> To be consistent with function naming scheme, where "ctrl" is the
>> old API, and "control" the new, you could call the start functions
>> something with "ctrl" and "control" as well.
> 
> i'll make this change in v3.
> 
>>
>> Maybe it's worth mentioning that fact somewhere in the beginning of
>> the file, as a comment (i.e., that "ctrl" denotes the old API).
> 
> i'll make this change in v3.
> 
>>
>>> +	} u;
>>>   	void *arg;
>>>   	int ret;
>>
>> Why is 'ret' needed? (This question is unrelated to your patch.)
> 
> i'm not the original author so difficult to answer authoritatively. but
> if i have to speculate i'd say it might be to work around the windows
> pthread_join stub being implemented as a noop. specifically it didn't
> communicate the return value from the start_routine.
> 
> the recently added rte_thread_join addresses this (which
> rte_control_thread_create uses) and could remove ret parameter and to
> avoid touching the new function implementation in the future it can not
> use ret now.
> 
> i'll make this change in v3.
> 
> for the original function i will leave the code as is to minimize the
> diff. when rte_ctrl_thread_create is removed i'll make a note to
> eliminate the ret parameter as well.
> 

I would focus on minimizing the complexity of the end result, rather 
than the size of the patch.

>>
>>>   	/* Control thread status.
>>> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>>>   	enum __rte_ctrl_thread_status ctrl_thread_status;
>>>   };
>>> -static void *ctrl_thread_init(void *arg)
>>> +static int ctrl_thread_init(void *arg)
>>>   {
>>>   	struct internal_config *internal_conf =
>>>   		eal_get_internal_configuration();
>>>   	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>>>   	struct rte_thread_ctrl_params *params = arg;
>>> -	void *(*start_routine)(void *) = params->start_routine;
>>> -	void *routine_arg = params->arg;
>>>   	__rte_thread_init(rte_lcore_id(), cpuset);
>>>   	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
>>> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>>>   	if (params->ret != 0) {
>>>   		__atomic_store_n(&params->ctrl_thread_status,
>>>   			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
>>> -		return NULL;
>>> +		return params->ret;
>>>   	}
>>>   	__atomic_store_n(&params->ctrl_thread_status,
>>>   		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>>> -	return start_routine(routine_arg);
>>> +	return 0;
>>> +}
>>> +
>>> +static void *ctrl_thread_start(void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params = arg;
>>> +	void *(*start_routine)(void *) = params->u.start_routine;
>>> +
>>> +	if (ctrl_thread_init(arg) != 0)
>>> +		return NULL;
>>> +
>>> +	return start_routine(params->arg);
>>> +}
>>> +
>>> +static uint32_t control_thread_start(void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params = arg;
>>> +	rte_thread_func start_routine = params->u.thread_func;
>>> +
>>> +	if (ctrl_thread_init(arg) != 0)
>>> +		return params->ret;
>>> +
>>> +	return start_routine(params->arg);
>>>   }
>>>   int
>>> @@ -280,12 +303,12 @@ static void *ctrl_thread_init(void *arg)
>>>   	if (!params)
>>>   		return -ENOMEM;
>>> -	params->start_routine = start_routine;
>>> +	params->u.start_routine = start_routine;
>>>   	params->arg = arg;
>>>   	params->ret = 0;
>>>   	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> -	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
>>> +	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
>>>   	if (ret != 0) {
>>>   		free(params);
>>>   		return -ret;
>>> @@ -322,6 +345,60 @@ static void *ctrl_thread_init(void *arg)
>>>   }
>>>   int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> +	const rte_thread_attr_t *attr,
>>> +	rte_thread_func start_routine, void *arg)
>>> +{
>>> +	struct rte_thread_ctrl_params *params;
>>> +	enum __rte_ctrl_thread_status ctrl_thread_status;
>>> +	int ret;
>>> +
>>> +	params = malloc(sizeof(*params));
>>> +	if (!params)
>>
>> params == NULL
> 
> copied from original code, i'll fix the style in the new function to
> comply with the dpdk coding standard.
> 
> i'll fix in v3.
> 
>>
>>> +		return -ENOMEM;
>>> +
>>> +	params->u.thread_func = start_routine;
>>> +	params->arg = arg;
>>> +	params->ret = 0;
>>> +	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
>>> +
>>> +	ret = rte_thread_create(thread, attr, control_thread_start, params);
>>> +	if (ret != 0) {
>>> +		free(params);
>>> +		return -ret;
>>> +	}
>>> +
>>> +	if (name != NULL) {
>>> +		ret = rte_thread_setname((pthread_t)thread->opaque_id, name);
>>> +		if (ret < 0)
>>> +			RTE_LOG(DEBUG, EAL,
>>> +				"Cannot set name for ctrl thread\n");
>>> +	}
>>> +
>>> +	/* Wait for the control thread to initialize successfully */
>>> +	while ((ctrl_thread_status =
>>> +			__atomic_load_n(&params->ctrl_thread_status,
>>> +			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
>>> +		/* Yield the CPU. Using sched_yield call requires maintaining
>>> +		 * another implementation for Windows as sched_yield is not
>>> +		 * supported on Windows.
>>> +		 */
>>
>> sched_yield() also doesn't necessarily yield the CPU.
> 
> copied from original code and understood, but are you requesting a
> change here or just a comment correction? can you offer wording you
> would find suitable?
> 

I would just remove the comment.

"Yield the CPU. sched_yield() may seem like a natural choice here, but 
does not guarantee that a context switch will actually occur, and also 
does not exist on Windows."


>>
>>> +		rte_delay_us_sleep(1);
>>> +	}
>>> +
>>> +	/* Check if the control thread encountered an error */
>>> +	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
>>> +		/* ctrl thread is exiting */
>>> +		rte_thread_join(*thread, NULL);
>>> +	}
>>> +
>>> +	ret = params->ret;
>>> +	free(params);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int
>>>   rte_thread_register(void)
>>>   {
>>>   	unsigned int lcore_id;
>>> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
>>> index b9edf70..ae7afbe 100644
>>> --- a/lib/eal/include/rte_thread.h
>>> +++ b/lib/eal/include/rte_thread.h
>>> @@ -95,6 +95,35 @@ int rte_thread_create(rte_thread_t *thread_id,
>>>   		rte_thread_func thread_func, void *arg);
>>>   /**
>>> + * Create a control thread.
>>> + *
>>> + * Creates a control thread with the given name and attributes. The
>>> + * affinity of the new thread is based on the CPU affinity retrieved
>>> + * at the time rte_eal_init() was called, the dataplane and service
>>> + * lcores are then excluded. If setting the name of the thread fails,
>>
>> "the EAL threads are then excluded"
> 
> i'll modify in v3.
> 
>>
>>> + * the error is ignored and a debug message is logged.
>>> + *
>>> + * @param thread
>>> + *   Filled with the thread id of the new created thread.
>>> + * @param name
>>> + *   The name of the control thread (max 16 characters including '\0').
>>
>> Is there a constant for this limit?
> 
> i have a series introducing rte_lcore_{set,get}_name api that introduces
> a constant (probably i'll post it today). as of this series there is no
> constant.
> 
>>
>>> + * @param thread_attr
>>> + *   Attributes for the new thread.
>>> + * @param thread_func
>>> + *   Function to be executed by the new thread.
>>> + * @param arg
>>> + *   Argument passed to start_routine.
>>> + * @return
>>> + *   On success, returns 0; on error, it returns a negative value
>>> + *   corresponding to the error number.
>>
>> List the possible error codes.
> 
> i would like to see the range of codes be part of the api & abi i've
> received resistance to the idea. for now i'll nack this comment which
> leaves it consistent with other apis documentation.
>>
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_control_thread_create(rte_thread_t *thread, const char *name,
>>> +		const rte_thread_attr_t *thread_attr,
>>> +		rte_thread_func thread_func, void *arg);
>>> +
>>> +/**
>>>    * @warning
>>>    * @b EXPERIMENTAL: this API may change without prior notice.
>>>    *
>>> diff --git a/lib/eal/version.map b/lib/eal/version.map
>>> index 7ad12a7..8f9eeb9 100644
>>> --- a/lib/eal/version.map
>>> +++ b/lib/eal/version.map
>>> @@ -440,6 +440,9 @@ EXPERIMENTAL {
>>>   	rte_thread_detach;
>>>   	rte_thread_equal;
>>>   	rte_thread_join;
>>> +
>>> +	# added in 23.03
>>> +	rte_control_thread_create;
>>>   };
>>>   INTERNAL {