Re: [dpdk-dev] [PATCH v6 02/10] eal: add thread attributes

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

2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> Implement thread attributes for:
> * thread affinity
> * thread priority
> 
> Implement functions for managing thread attributes.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/librte_eal/common/rte_thread.c            | 53 ++++++++++++
>  lib/librte_eal/include/rte_thread.h           | 82 +++++++++++++++++++
>  lib/librte_eal/include/rte_thread_types.h     |  3 +
>  .../include/rte_windows_thread_types.h        |  3 +
>  lib/librte_eal/windows/rte_thread.c           | 56 +++++++++++++
>  5 files changed, 197 insertions(+)
> 
> diff --git a/lib/librte_eal/common/rte_thread.c b/lib/librte_eal/common/rte_thread.c
> index 5ec382949..0bd1b115d 100644
> --- a/lib/librte_eal/common/rte_thread.c
> +++ b/lib/librte_eal/common/rte_thread.c

Please see a comment to Windows counterpart of this file.

[...]
> diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
> index cbc07f739..bfdd8e1b1 100644
> --- a/lib/librte_eal/include/rte_thread.h
> +++ b/lib/librte_eal/include/rte_thread.h
> @@ -28,6 +28,19 @@ extern "C" {
>  #include <rte_thread_types.h>
>  #endif
>  
> +enum rte_thread_priority {
> +	RTE_THREAD_PRIORITY_NORMAL            = EAL_THREAD_PRIORITY_NORMAL,
> +	RTE_THREAD_PRIORITY_REALTIME_CRITICAL = EAL_THREAD_PRIORITY_REALTIME_CIRTICAL,

Please add Doxygen comments for each new public item in the API.

> +	/*
> +	 * This enum can be extended to allow more priority levels.
> +	 */

This comment is useless: any enum can be extended.
(FYI, recently DPDK eliminated xxx_MAX enum members
just so that apps don't rely on enums not being extended.)

> +};
> +
> +typedef struct {
> +	enum rte_thread_priority priority;
> +	rte_cpuset_t cpuset;
> +} rte_thread_attr_t;
> +
>  /**
>   * TLS key type, an opaque pointer.
>   */
> @@ -60,6 +73,75 @@ rte_thread_t rte_thread_self(void);
>  __rte_experimental
>  int rte_thread_equal(rte_thread_t t1, rte_thread_t t2);
>  
> +/**
> + * Initialize the attributes of a thread.
> + * These attributes can be passed to the rte_thread_create() function
> + * that will create a new thread and set its attributes according to attr;

Typo: ";" -> "."

> + *
> + * @param attr
> + *   Thread attributes to initialize.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_attr_init(rte_thread_attr_t *attr);
> +
> +/**
> + * Set the CPU affinity value in the thread attributes pointed to
> + * by 'thread_attr'.
> + *
> + * @param thread_attr
> + *   Points to the thread attributes in which affinity will be updated.
> + *
> + * @param cpuset
> + *   Points to the value of the affinity to be set.
> + *
> + * @return
> + *   On success, return 0.
> + *   On failure, return a positive errno-style error number.
> + */
> +__rte_experimental
> +int rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
> +				 rte_cpuset_t *cpuset);

This description belongs to another function, doesn't it?
Same for other added functions below. Please double-check comments.

[...]
> diff --git a/lib/librte_eal/include/rte_thread_types.h b/lib/librte_eal/include/rte_thread_types.h
> index 19fb85e38..a884daf17 100644
> --- a/lib/librte_eal/include/rte_thread_types.h
> +++ b/lib/librte_eal/include/rte_thread_types.h
> @@ -7,6 +7,9 @@
>  
>  #include <pthread.h>
>  
> +#define EAL_THREAD_PRIORITY_NORMAL               0
> +#define EAL_THREAD_PRIORITY_REALTIME_CIRTICAL    99

Typo: "cirtical" -> "critical"
Are these values chosen arbitrarily?
What do you think of making "enum rte_thread_priority" members just 0 and 1,
then mapping them to OS-specific values in getters/setters?

> +
>  typedef pthread_t                       rte_thread_t;
>  
>  #endif /* _RTE_THREAD_TYPES_H_ */
> diff --git a/lib/librte_eal/windows/include/rte_windows_thread_types.h b/lib/librte_eal/windows/include/rte_windows_thread_types.h
> index ebd3d9e8f..8cb4b3856 100644
> --- a/lib/librte_eal/windows/include/rte_windows_thread_types.h
> +++ b/lib/librte_eal/windows/include/rte_windows_thread_types.h
> @@ -7,6 +7,9 @@
>  
>  #include <rte_windows.h>
>  
> +#define EAL_THREAD_PRIORITY_NORMAL             THREAD_PRIORITY_NORMAL
> +#define EAL_THREAD_PRIORITY_REALTIME_CIRTICAL  THREAD_PRIORITY_TIME_CRITICAL
> +
>  typedef DWORD                       rte_thread_t;
>  
>  #endif /* _RTE_THREAD_TYPES_H_ */
> diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c
> index 940d9c653..b29336cbd 100644
> --- a/lib/librte_eal/windows/rte_thread.c
> +++ b/lib/librte_eal/windows/rte_thread.c
> @@ -24,6 +24,62 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
>  	return t1 == t2 ? 1 : 0;
>  }
>  
> +int
> +rte_thread_attr_init(rte_thread_attr_t *attr)
> +{
> +	if (attr == NULL) {
> +		RTE_LOG(DEBUG, EAL,
> +		"Unable to init thread attributes, invalid parameter\n");
> +		return EINVAL;
> +	}

This message doesn't add value for debugging: caller already knows that
attribute initialization failed (that's what function attempts to do) and
that the parameter is invalid (EINVAL).
I'd remove it (same applies below).
If you find it useful to keep, an extra indent missing (also more below).

[...]