[v6,02/10] eal: add thread attributes
Checks
Commit Message
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(+)
Comments
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).
[...]
29/04/2021 02:50, Dmitry Kozlyuk:
> 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> > +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).
Recently in ethdev we added more messages like this for NULL parameters.
I agree it is not a lot useful but I understand that lazy developers may like it ;)
-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net>
Sent: Thursday, April 29, 2021 12:48 AM
29/04/2021 02:50, Dmitry Kozlyuk:
> > 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> > > +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).
> Recently in ethdev we added more messages like this for NULL parameters.
> I agree it is not a lot useful but I understand that lazy developers may like it ;)
Shouldn't this specific case be an assert? Unless we are trying to maintain compatibility with existing badly designed semantics.
The whole calling pattern is non-sensible, the caller passes an NULL parameter to a function where the input contract is non-NULL and then proceeds to handle the error by doing what that could possibly be useful exactly?
2021-04-29 09:48 (UTC+0200), Thomas Monjalon:
> 29/04/2021 02:50, Dmitry Kozlyuk:
> > 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> > > +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).
>
> Recently in ethdev we added more messages like this for NULL parameters.
> I agree it is not a lot useful but I understand that lazy developers may like it ;)
Messages in 53ef1b34776b ("ethdev: add sanity checks in control APIs")
at least tell why exactly a parameter is invalid and which parameter is it.
2021-04-29 15:52 (UTC+0000), Tyler Retzlaff:
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, April 29, 2021 12:48 AM
>
>
> 29/04/2021 02:50, Dmitry Kozlyuk:
> > > 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile:
> > > > +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).
>
> > Recently in ethdev we added more messages like this for NULL parameters.
> > I agree it is not a lot useful but I understand that lazy developers may like it ;)
>
> Shouldn't this specific case be an assert? Unless we are trying to maintain compatibility with existing badly designed semantics.
>
> The whole calling pattern is non-sensible, the caller passes an NULL parameter to a function where the input contract is non-NULL and then proceeds to handle the error by doing what that could possibly be useful exactly?
+1 in this case.
This function is only specified to diagnose ENOMEM. On my Linux machine
pthread_attr_init(NULL) crashes, I guess that would be compatible behavior.
@@ -29,6 +29,59 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
return pthread_equal(t1, t2);
}
+int
+rte_thread_attr_init(rte_thread_attr_t *attr)
+{
+ if (attr == NULL) {
+ RTE_LOG(DEBUG, EAL, "Invalid thread attributes parameter\n");
+ return EINVAL;
+ }
+
+ CPU_ZERO(&attr->cpuset);
+ attr->priority = RTE_THREAD_PRIORITY_NORMAL;
+
+ return 0;
+}
+
+int
+rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
+ rte_cpuset_t *cpuset)
+{
+ if (thread_attr == NULL || cpuset == NULL) {
+ RTE_LOG(DEBUG, EAL, "Invalid thread attributes parameter\n");
+ return EINVAL;
+ }
+ thread_attr->cpuset = *cpuset;
+ return 0;
+}
+
+int
+rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
+ rte_cpuset_t *cpuset)
+{
+ if ((thread_attr == NULL) || (cpuset == NULL)) {
+ RTE_LOG(DEBUG, EAL, "Invalid thread attributes parameter\n");
+ return EINVAL;
+ }
+
+ *cpuset = thread_attr->cpuset;
+ return 0;
+}
+
+int
+rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
+ enum rte_thread_priority priority)
+{
+ if (thread_attr == NULL) {
+ RTE_LOG(DEBUG, EAL,
+ "Unable to set priority attribute, invalid parameter\n");
+ return EINVAL;
+ }
+
+ thread_attr->priority = priority;
+ return 0;
+}
+
int
rte_thread_key_create(rte_thread_key *key, void (*destructor)(void *))
{
@@ -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,
+ /*
+ * This enum can be extended to allow more priority levels.
+ */
+};
+
+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;
+ *
+ * @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);
+
+/**
+ * Get the value of CPU affinity that is set in the thread attributes pointed
+ * to by 'thread_attr'.
+ *
+ * @param thread_attr
+ * Points to the thread attributes from which affinity will be retrieved.
+ *
+ * @param cpuset
+ * Pointer to the memory that will store the affinity.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
+ rte_cpuset_t *cpuset);
+
+/**
+ * Set the thread priority value in the thread attributes pointed to
+ * by 'thread_attr'.
+ *
+ * @param thread_attr
+ * Points to the thread attributes in which priority will be updated.
+ *
+ * @param priority
+ * Points to the value of the priority to be set.
+ *
+ * @return
+ * On success, return 0.
+ * On failure, return a positive errno-style error number.
+ */
+__rte_experimental
+int rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
+ enum rte_thread_priority priority);
+
/**
* Set core affinity of the current thread.
* Support both EAL and non-EAL thread and update TLS.
@@ -7,6 +7,9 @@
#include <pthread.h>
+#define EAL_THREAD_PRIORITY_NORMAL 0
+#define EAL_THREAD_PRIORITY_REALTIME_CIRTICAL 99
+
typedef pthread_t rte_thread_t;
#endif /* _RTE_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_ */
@@ -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;
+ }
+
+ attr->priority = RTE_THREAD_PRIORITY_NORMAL;
+ CPU_ZERO(&attr->cpuset);
+ return 0;
+}
+
+int
+rte_thread_attr_set_affinity(rte_thread_attr_t *thread_attr,
+ rte_cpuset_t *cpuset)
+{
+ if (thread_attr == NULL) {
+ RTE_LOG(DEBUG, EAL,
+ "Unable to set affinity attribute, invalid parameter\n");
+ return EINVAL;
+ }
+
+ thread_attr->cpuset = *cpuset;
+ return 0;
+}
+
+int
+rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
+ rte_cpuset_t *cpuset)
+{
+ if (thread_attr == NULL) {
+ RTE_LOG(DEBUG, EAL,
+ "Unable to set affinity attribute, invalid parameter\n");
+ return EINVAL;
+ }
+
+ *cpuset = thread_attr->cpuset;
+ return 0;
+}
+
+int
+rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
+ enum rte_thread_priority priority)
+{
+ if (thread_attr == NULL) {
+ RTE_LOG(DEBUG, EAL,
+ "Unable to set priority attribute, invalid parameter\n");
+ return EINVAL;
+ }
+
+ thread_attr->priority = priority;
+ return 0;
+}
+
int
rte_thread_key_create(rte_thread_key *key,
__rte_unused void (*destructor)(void *))