Re: [dpdk-dev] [PATCH v9 01/10] eal: add thread id and simple thread functions

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

2021-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Use a portable, type-safe representation for the thread identifier.
> Add functions for comparing thread ids and obtaining the thread id
> for the current thread.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/eal/common/rte_thread.c                   | 105 ++++++++++++++++++
>  lib/eal/include/rte_thread.h                  |  53 +++++++--
>  lib/eal/include/rte_thread_types.h            |  10 ++
>  .../include/rte_windows_thread_types.h        |  10 ++
>  lib/eal/windows/rte_thread.c                  |  17 +++
>  5 files changed, 186 insertions(+), 9 deletions(-)
>  create mode 100644 lib/eal/common/rte_thread.c
>  create mode 100644 lib/eal/include/rte_thread_types.h
>  create mode 100644 lib/eal/windows/include/rte_windows_thread_types.h

It is strange that new files are being filled in the series, but are neither
compiled nor installed until the last patch. Any reason not to replace
lib/eal/unix/rte_thread.c starting from this patch?

A better name for rte_thread_types.h would be rte_posix_thread_types.h
to indicate this file is not really common.

> 
> diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> new file mode 100644
> index 0000000000..1292f7a8f8
> --- /dev/null
> +++ b/lib/eal/common/rte_thread.c
> @@ -0,0 +1,105 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 Mellanox Technologies, Ltd
> + * Copyright(c) 2021 Microsoft Corporation
> + */
> +
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_log.h>
> +#include <rte_thread.h>
> +
> +struct eal_tls_key {
> +	pthread_key_t thread_index;
> +};
> +
> +rte_thread_t
> +rte_thread_self(void)
> +{
> +	rte_thread_t thread_id = { 0 };

(Applies to entire series.)
Please do not initialize variables when you intend to overwrite them.
This prevents compiler from reporting code paths where the variable is used
before a proper assignment.

> +
> +	thread_id.opaque_id = pthread_self();
> +
> +	return thread_id;
> +}
> +

[...]
> diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
> index 8be8ed8f36..347df1a6ae 100644
> --- a/lib/eal/include/rte_thread.h
> +++ b/lib/eal/include/rte_thread.h
> @@ -1,6 +1,8 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2021 Mellanox Technologies, Ltd
> + * Copyright(c) 2021 Microsoft Corporation
>   */
> +#include <stdint.h>
>  
>  #include <rte_os.h>
>  #include <rte_compat.h>
> @@ -20,11 +22,50 @@
>  extern "C" {
>  #endif
>  
> +#include <sched.h>
> +#if defined(RTE_USE_WINDOWS_THREAD_TYPES)

Redundant braces are discouraged in DPDK codebase.

[...]
> diff --git a/lib/eal/include/rte_thread_types.h b/lib/eal/include/rte_thread_types.h
> new file mode 100644
> index 0000000000..d67b24a563
> --- /dev/null
> +++ b/lib/eal/include/rte_thread_types.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Microsoft Corporation
> + */
> +
> +#ifndef _RTE_THREAD_TYPES_H_
> +#define _RTE_THREAD_TYPES_H_
> +
> +#include <pthread.h>
> +
> +#endif /* _RTE_THREAD_TYPES_H_ */
> diff --git a/lib/eal/windows/include/rte_windows_thread_types.h b/lib/eal/windows/include/rte_windows_thread_types.h
> new file mode 100644
> index 0000000000..60e6d94553
> --- /dev/null
> +++ b/lib/eal/windows/include/rte_windows_thread_types.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Microsoft Corporation
> + */
> +
> +#ifndef _RTE_THREAD_TYPES_H_
> +#define _RTE_THREAD_TYPES_H_
> +
> +#include <rte_windows.h>
> +
> +#endif /* _RTE_THREAD_TYPES_H_ */

Thread types headers should have a check that forbids including them
directly, see lib/eal/$arch/rte_atomic_64.h for examples.

Note that rte_*_thread_types.h should be `indirect_headers`, not `headers`
in `meson.build` so that they're not checked by `buildtools/chkincs` when is
gets enabled for Windows. This is especially
true for lib/eal/common/rte_thread.h that cannot work without pthread.

However, in later patches these headers only contain mutex bits,
see the comment to pathc 07/10 about them. Maybe we don't need these files
after all.