Re: [PATCH v18 2/8] eal: add thread attributes

Re: [PATCH v18 2/8] eal: add thread attributes

[Ananyev, Konstantin]

> Implement thread attributes for:
> * thread affinity
> * thread priority
> Implement functions for managing thread attributes.
> 
> Priority is represented through an enum that allows for two levels:
> 	- RTE_THREAD_PRIORITY_NORMAL
> 	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL
> 
> Affinity is described by the rte_cpuset_t type.
> 
> An rte_thread_attr_t object can be set to the default values
> by calling rte_thread_attr_init().
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  lib/eal/common/rte_thread.c  | 46 ++++++++++++++++++
>  lib/eal/include/rte_thread.h | 91 ++++++++++++++++++++++++++++++++++++
>  lib/eal/version.map          |  4 ++
>  lib/eal/windows/rte_thread.c | 44 +++++++++++++++++
>  4 files changed, 185 insertions(+)
> 
> diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> index 92a7451b0a..27ad1c7eb0 100644
> --- a/lib/eal/common/rte_thread.c
> +++ b/lib/eal/common/rte_thread.c
> @@ -9,6 +9,7 @@
>  #include <string.h>
> 
>  #include <rte_common.h>
> +#include <rte_debug.h>
>  #include <rte_errno.h>
>  #include <rte_log.h>
>  #include <rte_thread.h>
> @@ -33,6 +34,51 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
>  	return pthread_equal((pthread_t)t1.opaque_id, (pthread_t)t2.opaque_id);
>  }
> 
> +int
> +rte_thread_attr_init(rte_thread_attr_t *attr)
> +{
> +	RTE_VERIFY(attr != NULL);

As a generic one, here and everywhere:
Please don't use RTE_VERIFY() for checking input function parameters.
We don't want to panic in case of just invalid parameter from user.

> +
> +	CPU_ZERO(&attr->cpuset);
> +	attr->priority = RTE_THREAD_PRIORITY_NORMAL;
> +
> +	return 0;
> +}
> +

Re: [PATCH v18 2/8] eal: add thread attributes

[Tyler Retzlaff]

On Fri, Feb 04, 2022 at 07:21:10PM +0000, Ananyev, Konstantin wrote:
> > Implement thread attributes for:
> > * thread affinity
> > * thread priority
> > Implement functions for managing thread attributes.
> > 
> > Priority is represented through an enum that allows for two levels:
> > 	- RTE_THREAD_PRIORITY_NORMAL
> > 	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL
> > 
> > Affinity is described by the rte_cpuset_t type.
> > 
> > An rte_thread_attr_t object can be set to the default values
> > by calling rte_thread_attr_init().
> > 
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> > ---
> >  lib/eal/common/rte_thread.c  | 46 ++++++++++++++++++
> >  lib/eal/include/rte_thread.h | 91 ++++++++++++++++++++++++++++++++++++
> >  lib/eal/version.map          |  4 ++
> >  lib/eal/windows/rte_thread.c | 44 +++++++++++++++++
> >  4 files changed, 185 insertions(+)
> > 
> > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> > index 92a7451b0a..27ad1c7eb0 100644
> > --- a/lib/eal/common/rte_thread.c
> > +++ b/lib/eal/common/rte_thread.c
> > @@ -9,6 +9,7 @@
> >  #include <string.h>
> > 
> >  #include <rte_common.h>
> > +#include <rte_debug.h>
> >  #include <rte_errno.h>
> >  #include <rte_log.h>
> >  #include <rte_thread.h>
> > @@ -33,6 +34,51 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
> >  	return pthread_equal((pthread_t)t1.opaque_id, (pthread_t)t2.opaque_id);
> >  }
> > 
> > +int
> > +rte_thread_attr_init(rte_thread_attr_t *attr)
> > +{
> > +	RTE_VERIFY(attr != NULL);
> 
> As a generic one, here and everywhere:
> Please don't use RTE_VERIFY() for checking input function parameters.
> We don't want to panic in case of just invalid parameter from user.

i ask this question again. what useful thing will the user application
do when handling -EINVAL or rte_errno = EINVAL is returned for
incorrectly supplied parameters?

again, there should be a mechanism that allows a policy for how these
non-recoverable errors are handled rather than defaulting to tossing 
it over the fence and expecting the application to do something 
sensible when the only thing it could do is conclusively more
complicated than having passed the correct parameters in the first place.

more often then not application programmers will ignore superfluous
return values from functions like this resulting in the bug remaining
longer and the state / reason being lost.

please reconsider.

Re: [PATCH v18 2/8] eal: add thread attributes

[Thomas Monjalon]

05/02/2022 05:41, Tyler Retzlaff:
> On Fri, Feb 04, 2022 at 07:21:10PM +0000, Ananyev, Konstantin wrote:
> > > +int
> > > +rte_thread_attr_init(rte_thread_attr_t *attr)
> > > +{
> > > +	RTE_VERIFY(attr != NULL);
> > 
> > As a generic one, here and everywhere:
> > Please don't use RTE_VERIFY() for checking input function parameters.
> > We don't want to panic in case of just invalid parameter from user.
> 
> i ask this question again. what useful thing will the user application
> do when handling -EINVAL or rte_errno = EINVAL is returned for
> incorrectly supplied parameters?
> 
> again, there should be a mechanism that allows a policy for how these
> non-recoverable errors are handled rather than defaulting to tossing 
> it over the fence and expecting the application to do something 
> sensible when the only thing it could do is conclusively more
> complicated than having passed the correct parameters in the first place.
> 
> more often then not application programmers will ignore superfluous
> return values from functions like this resulting in the bug remaining
> longer and the state / reason being lost.
> 
> please reconsider.

The application should just abort this feature indeed.
But remember the application can have other features.
In some applications, the DPDK features are a minor part.
So we don't want to crash the entire application just because
a DPDK feature has a bug.
More generally, a library should never crash an entire application.

RE: [PATCH v18 2/8] eal: add thread attributes

[Ananyev, Konstantin]

> 
> On Fri, Feb 04, 2022 at 07:21:10PM +0000, Ananyev, Konstantin wrote:
> > > Implement thread attributes for:
> > > * thread affinity
> > > * thread priority
> > > Implement functions for managing thread attributes.
> > >
> > > Priority is represented through an enum that allows for two levels:
> > > 	- RTE_THREAD_PRIORITY_NORMAL
> > > 	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL
> > >
> > > Affinity is described by the rte_cpuset_t type.
> > >
> > > An rte_thread_attr_t object can be set to the default values
> > > by calling rte_thread_attr_init().
> > >
> > > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> > > ---
> > >  lib/eal/common/rte_thread.c  | 46 ++++++++++++++++++
> > >  lib/eal/include/rte_thread.h | 91 ++++++++++++++++++++++++++++++++++++
> > >  lib/eal/version.map          |  4 ++
> > >  lib/eal/windows/rte_thread.c | 44 +++++++++++++++++
> > >  4 files changed, 185 insertions(+)
> > >
> > > diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
> > > index 92a7451b0a..27ad1c7eb0 100644
> > > --- a/lib/eal/common/rte_thread.c
> > > +++ b/lib/eal/common/rte_thread.c
> > > @@ -9,6 +9,7 @@
> > >  #include <string.h>
> > >
> > >  #include <rte_common.h>
> > > +#include <rte_debug.h>
> > >  #include <rte_errno.h>
> > >  #include <rte_log.h>
> > >  #include <rte_thread.h>
> > > @@ -33,6 +34,51 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
> > >  	return pthread_equal((pthread_t)t1.opaque_id, (pthread_t)t2.opaque_id);
> > >  }
> > >
> > > +int
> > > +rte_thread_attr_init(rte_thread_attr_t *attr)
> > > +{
> > > +	RTE_VERIFY(attr != NULL);
> >
> > As a generic one, here and everywhere:
> > Please don't use RTE_VERIFY() for checking input function parameters.
> > We don't want to panic in case of just invalid parameter from user.
> 
> i ask this question again. what useful thing will the user application
> do when handling -EINVAL or rte_errno = EINVAL is returned for
> incorrectly supplied parameters?

Let the user application to decide.
But inside the lib we shouldn't just crash if user provided invalid parameters
for one of our functions.

> 
> again, there should be a mechanism that allows a policy for how these
> non-recoverable errors are handled rather than defaulting to tossing
> it over the fence and expecting the application to do something
> sensible when the only thing it could do is conclusively more
> complicated than having passed the correct parameters in the first place.
> 
> more often then not application programmers will ignore superfluous
> return values from functions like this resulting in the bug remaining
> longer and the state / reason being lost.
> 
> please reconsider.

Re: [PATCH v18 2/8] eal: add thread attributes

[Tyler Retzlaff]

On Sat, Feb 05, 2022 at 10:00:01AM +0100, Thomas Monjalon wrote:
> 05/02/2022 05:41, Tyler Retzlaff:
> > On Fri, Feb 04, 2022 at 07:21:10PM +0000, Ananyev, Konstantin wrote:
> > > > +int
> > > > +rte_thread_attr_init(rte_thread_attr_t *attr)
> > > > +{
> > > > +	RTE_VERIFY(attr != NULL);
> > > 
> > > As a generic one, here and everywhere:
> > > Please don't use RTE_VERIFY() for checking input function parameters.
> > > We don't want to panic in case of just invalid parameter from user.
> > 
> > i ask this question again. what useful thing will the user application
> > do when handling -EINVAL or rte_errno = EINVAL is returned for
> > incorrectly supplied parameters?
> > 
> > again, there should be a mechanism that allows a policy for how these
> > non-recoverable errors are handled rather than defaulting to tossing 
> > it over the fence and expecting the application to do something 
> > sensible when the only thing it could do is conclusively more
> > complicated than having passed the correct parameters in the first place.
> > 
> > more often then not application programmers will ignore superfluous
> > return values from functions like this resulting in the bug remaining
> > longer and the state / reason being lost.
> > 
> > please reconsider.
> 
> The application should just abort this feature indeed.
> But remember the application can have other features.
> In some applications, the DPDK features are a minor part.
> So we don't want to crash the entire application just because
> a DPDK feature has a bug.
> More generally, a library should never crash an entire application.
> 
> 

you haven't addressed my concern. when deployed at scale i need to be
able to capture the state of what was wrong in order to debug the
problem. a crash dump does this.

i agree that the dpdk functionality may be shared/hosted in a process
that does other work but this api hard-codes a behavior that does not
permit easy diagnostics of buggy usage of api vs actual errors. there
is a distinction and it is important.

with the api hard-coded to a single policy you leave the application
author only the option of wrapping every single api in some boilerplate
error handling logic that has to teadeously log part of the application
state (hopefully the part you need)  from the current scope before
explicitly exiting.

as i explained previously i'm not asking that dpdk hard-codes a policy
of crashing either. we just need it to be a choice rather than forcing a
single hard-coded behavior.

[PATCH v18 2/8] eal: add thread attributes

[Narcisa Ana Maria Vasile]

From: Narcisa Vasile <navasile@microsoft.com>

Implement thread attributes for:
* thread affinity
* thread priority
Implement functions for managing thread attributes.

Priority is represented through an enum that allows for two levels:
	- RTE_THREAD_PRIORITY_NORMAL
	- RTE_THREAD_PRIORITY_REALTIME_CRITICAL

Affinity is described by the rte_cpuset_t type.

An rte_thread_attr_t object can be set to the default values
by calling rte_thread_attr_init().

Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
 lib/eal/common/rte_thread.c  | 46 ++++++++++++++++++
 lib/eal/include/rte_thread.h | 91 ++++++++++++++++++++++++++++++++++++
 lib/eal/version.map          |  4 ++
 lib/eal/windows/rte_thread.c | 44 +++++++++++++++++
 4 files changed, 185 insertions(+)

diff --git a/lib/eal/common/rte_thread.c b/lib/eal/common/rte_thread.c
index 92a7451b0a..27ad1c7eb0 100644
--- a/lib/eal/common/rte_thread.c
+++ b/lib/eal/common/rte_thread.c
@@ -9,6 +9,7 @@
 #include <string.h>
 
 #include <rte_common.h>
+#include <rte_debug.h>
 #include <rte_errno.h>
 #include <rte_log.h>
 #include <rte_thread.h>
@@ -33,6 +34,51 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
 	return pthread_equal((pthread_t)t1.opaque_id, (pthread_t)t2.opaque_id);
 }
 
+int
+rte_thread_attr_init(rte_thread_attr_t *attr)
+{
+	RTE_VERIFY(attr != NULL);
+
+	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)
+{
+	RTE_VERIFY(thread_attr != NULL);
+	RTE_VERIFY(cpuset != NULL);
+
+	thread_attr->cpuset = *cpuset;
+
+	return 0;
+}
+
+int
+rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
+		rte_cpuset_t *cpuset)
+{
+	RTE_VERIFY(thread_attr != NULL);
+	RTE_VERIFY(cpuset != NULL);
+
+	*cpuset = thread_attr->cpuset;
+
+	return 0;
+}
+
+int
+rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
+		enum rte_thread_priority priority)
+{
+	RTE_VERIFY(thread_attr != NULL);
+
+	thread_attr->priority = priority;
+	return 0;
+}
+
 int
 rte_thread_key_create(rte_thread_key *key, void (*destructor)(void *))
 {
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index c9cdeb07aa..8a20215a94 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -31,6 +31,28 @@ typedef struct rte_thread_tag {
 	uintptr_t opaque_id; /**< thread identifier */
 } rte_thread_t;
 
+/**
+ * Thread priority values.
+ */
+enum rte_thread_priority {
+	RTE_THREAD_PRIORITY_NORMAL            = 0,
+	/**< normal thread priority, the default */
+	RTE_THREAD_PRIORITY_REALTIME_CRITICAL = 1,
+	/**< highest thread priority allowed */
+};
+
+#ifdef RTE_HAS_CPUSET
+
+/**
+ * Representation for thread attributes.
+ */
+typedef struct {
+	enum rte_thread_priority priority; /**< thread priority */
+	rte_cpuset_t cpuset; /**< thread affinity */
+} rte_thread_attr_t;
+
+#endif /* RTE_HAS_CPUSET */
+
 /**
  * TLS key type, an opaque pointer.
  */
@@ -63,6 +85,75 @@ int rte_thread_equal(rte_thread_t t1, rte_thread_t t2);
 
 #ifdef RTE_HAS_CPUSET
 
+/**
+ * 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.
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 24e9747795..3fc33fcd70 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -423,6 +423,10 @@ EXPERIMENTAL {
 
 	rte_thread_self;
 	rte_thread_equal;
+	rte_thread_attr_init;
+	rte_thread_attr_get_affinity;
+	rte_thread_attr_set_affinity;
+	rte_thread_attr_set_priority;
 };
 
 INTERNAL {
diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c
index 26c8dd4ebe..f65919f46d 100644
--- a/lib/eal/windows/rte_thread.c
+++ b/lib/eal/windows/rte_thread.c
@@ -4,6 +4,7 @@
  */
 
 #include <rte_common.h>
+#include <rte_debug.h>
 #include <rte_errno.h>
 #include <rte_thread.h>
 #include <rte_windows.h>
@@ -28,6 +29,49 @@ rte_thread_equal(rte_thread_t t1, rte_thread_t t2)
 	return t1.opaque_id == t2.opaque_id;
 }
 
+int
+rte_thread_attr_init(rte_thread_attr_t *attr)
+{
+	RTE_VERIFY(attr != NULL);
+
+	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)
+{
+	RTE_VERIFY(thread_attr != NULL);
+	thread_attr->cpuset = *cpuset;
+
+	return 0;
+}
+
+int
+rte_thread_attr_get_affinity(rte_thread_attr_t *thread_attr,
+		rte_cpuset_t *cpuset)
+{
+	RTE_VERIFY(thread_attr != NULL);
+
+	*cpuset = thread_attr->cpuset;
+
+	return 0;
+}
+
+int
+rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr,
+		enum rte_thread_priority priority)
+{
+	RTE_VERIFY(thread_attr != NULL);
+
+	thread_attr->priority = priority;
+
+	return 0;
+}
+
 int
 rte_thread_key_create(rte_thread_key *key,
 		__rte_unused void (*destructor)(void *))
-- 
2.31.0.vfs.0.1