Re: [PATCH v2 2/6] eal: add thread lifetime management

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2022-06-14 16:47 (UTC-0700), Tyler Retzlaff:
> On Windows, the function executed by a thread when the thread starts is
> represeneted by a function pointer of type DWORD (*func) (void*).
> On other platforms, the function pointer is a void* (*func) (void*).
> 
> Performing a cast between these two types of function pointers to
> uniformize the API on all platforms may result in undefined behavior.
> TO fix this issue, a wrapper that respects the signature required by
> CreateThread() has been created on Windows.

The interface issue is still there:
`rte_thread_func` allows the thread routine to have a pointer-sized result.
`rte_thread_join()` allows to obtain this value as `unsigned long`,
which is pointer-sized on 32-bit platforms
and less than that on 64-bit platforms.
This can lead to issues when developers assume they can return a pointer
and this works on 32 bits, but doesn't work on 64 bits.
If you want to keep API as close to phtread as possible,
the limitation must be at least clearly documented in Doxygen
(`rte_thread_func` is undocumented BTW).
I also suggest using `uint32_t` instead of `unsigned long`
precisely to avoid "is it pointer-sized?" doubts.
(I don't see much benefit in keeping pthread-like signature.
When moving from pthread to rte_thread,
it is as trivial to change the thread routine return type.)

> +int
> +rte_thread_create(rte_thread_t *thread_id,
> +		const rte_thread_attr_t *thread_attr,
> +		rte_thread_func thread_func, void *args)
> +{
> [...]
> +		if (thread_attr->priority ==
> +				RTE_THREAD_PRIORITY_REALTIME_CRITICAL) {
> +			ret = ENOTSUP;
> +			goto cleanup;
> +		}
> +		ret = thread_map_priority_to_os_value(thread_attr->priority,
> +				&param.sched_priority, &policy);
> +		if (ret != 0)
> +			goto cleanup;

thread_map_priority_to_os_value() already checks for unsupported values,
why not let it do this particular check?

> +int
> +rte_thread_join(rte_thread_t thread_id, unsigned long *value_ptr)
> +{
> +	int ret = 0;
> +	void *res = NULL;
> +	void **pres = NULL;
> +
> +	if (value_ptr != NULL)
> +		pres = &res;
> +
> +	ret = pthread_join((pthread_t)thread_id.opaque_id, pres);
> +	if (ret != 0) {
> +		RTE_LOG(DEBUG, EAL, "pthread_join failed\n");
> +		return ret;
> +	}
> +
> +	if (value_ptr != NULL && *pres != NULL)
> +		*value_ptr = *(unsigned long *)(*pres);
> +
> +	return 0;
> +}

What makes *pres == NULL special?

> +static DWORD
> +thread_func_wrapper(void *args)
> +{
> +	struct thread_routine_ctx *pctx = args;
> +	struct thread_routine_ctx ctx;
> +
> +	ctx.thread_func = pctx->thread_func;
> +	ctx.routine_args = pctx->routine_args;

ctx = *pctx?

> +
> +	free(pctx);
> +
> +	return (DWORD)(uintptr_t)ctx.thread_func(ctx.routine_args);
> +}
> +
> +int
> +rte_thread_create(rte_thread_t *thread_id,
> +		  const rte_thread_attr_t *thread_attr,
> +		  rte_thread_func thread_func, void *args)
> +{
> +	int ret = 0;
> +	DWORD tid;
> +	HANDLE thread_handle = NULL;
> +	GROUP_AFFINITY thread_affinity;
> +	struct thread_routine_ctx *ctx = NULL;
> +
> +	ctx = calloc(1, sizeof(*ctx));
> +	if (ctx == NULL) {
> +		RTE_LOG(DEBUG, EAL, "Insufficient memory for thread context allocations\n");
> +		ret = ENOMEM;
> +		goto cleanup;
> +	}
> +	ctx->routine_args = args;
> +	ctx->thread_func = thread_func;
> +
> +	thread_handle = CreateThread(NULL, 0, thread_func_wrapper, ctx,
> +		CREATE_SUSPENDED, &tid);
> +	if (thread_handle == NULL) {
> +		ret = thread_log_last_error("CreateThread()");
> +		free(ctx);
> +		goto cleanup;

Missing `free(ctx)` from other error paths below.

> +	}
> +	thread_id->opaque_id = tid;
> +
> +	if (thread_attr != NULL) {
> +		if (CPU_COUNT(&thread_attr->cpuset) > 0) {
> +			ret = convert_cpuset_to_affinity(
> +							&thread_attr->cpuset,
> +							&thread_affinity
> +							);
> +			if (ret != 0) {
> +				RTE_LOG(DEBUG, EAL, "Unable to convert cpuset to thread affinity\n");
> +				goto cleanup;
> +			}
> +
> +			if (!SetThreadGroupAffinity(thread_handle,
> +						    &thread_affinity, NULL)) {
> +				ret = thread_log_last_error("SetThreadGroupAffinity()");
> +				goto cleanup;
> +			}
> +		}
> +		ret = rte_thread_set_priority(*thread_id,
> +				thread_attr->priority);
> +		if (ret != 0) {
> +			RTE_LOG(DEBUG, EAL, "Unable to set thread priority\n");
> +			goto cleanup;
> +		}
> +	}
> +
> +	if (ResumeThread(thread_handle) == (DWORD)-1) {
> +		ret = thread_log_last_error("ResumeThread()");
> +		goto cleanup;
> +	}
> +
> +cleanup:
> +	if (thread_handle != NULL) {
> +		CloseHandle(thread_handle);
> +		thread_handle = NULL;
> +	}
> +
> +	return ret;
> +}