diff mbox series

[1/3] eal: add rte control thread create API

Message ID 1670271868-11364-2-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: rte_ctrl_thread_create API replacement |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Tyler Retzlaff Dec. 5, 2022, 8:24 p.m. UTC 
  Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
 lib/eal/include/rte_thread.h       | 29 ++++++++++++
 lib/eal/version.map                |  3 ++
 3 files changed, 117 insertions(+), 8 deletions(-)

Comments

Stephen Hemminger Dec. 5, 2022, 9:11 p.m. UTC | #1 
On Mon,  5 Dec 2022 12:24:26 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
>  lib/eal/include/rte_thread.h       | 29 ++++++++++++
>  lib/eal/version.map                |  3 ++
>  3 files changed, 117 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> index c5d8b43..ca85c51 100644
> --- a/lib/eal/common/eal_common_thread.c
> +++ b/lib/eal/common/eal_common_thread.c
> @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
>  };
>  
>  struct rte_thread_ctrl_params {
> -	void *(*start_routine)(void *);
> +	union {
> +		void * (*start_routine)(void *);
> +		rte_thread_func thread_func;
> +	} u;

Why not just use rte_thread_func, this in internal.

>  	void *arg;
>  	int ret;
>  	/* Control thread status.
> @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
>  	enum __rte_ctrl_thread_status ctrl_thread_status;
>  };
>  
> -static void *ctrl_thread_init(void *arg)
> +static int ctrl_thread_init(void *arg)
>  {
>  	struct internal_config *internal_conf =
>  		eal_get_internal_configuration();
>  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
>  	struct rte_thread_ctrl_params *params = arg;
> -	void *(*start_routine)(void *) = params->start_routine;
> -	void *routine_arg = params->arg;
>  
>  	__rte_thread_init(rte_lcore_id(), cpuset);
>  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
>  	if (params->ret != 0) {
>  		__atomic_store_n(&params->ctrl_thread_status,
>  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> -		return NULL;
> +		return params->ret;
>  	}
>  
>  	__atomic_store_n(&params->ctrl_thread_status,
>  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
>  
> -	return start_routine(routine_arg);
> +	return 0;
> +}
> +
> +static void *ctrl_thread_start(void *arg)
> +{
> +	struct rte_thread_ctrl_params *params = arg;
> +	void *(*start_routine)(void *) = params->u.start_routine;
> +
> +	if (0 != ctrl_thread_init(arg))
> +		return NULL;

DPDK uses the Linux not Windows coding style.
Windows uses the constant on left side of comparison because of the common
programming error of putting assignment where conditional was intended.
Linux uses a compiler that puts out a warning, so the more natural
style is action != result
Tyler Retzlaff Dec. 6, 2022, 12:21 a.m. UTC | #2 
On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index c5d8b43..ca85c51 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  
> >  struct rte_thread_ctrl_params {
> > -	void *(*start_routine)(void *);
> > +	union {
> > +		void * (*start_routine)(void *);
> > +		rte_thread_func thread_func;
> > +	} u;
> 
> Why not just use rte_thread_func, this in internal.
> 
> >  	void *arg;
> >  	int ret;
> >  	/* Control thread status.
> > @@ -243,14 +246,12 @@ struct rte_thread_ctrl_params {
> >  	enum __rte_ctrl_thread_status ctrl_thread_status;
> >  };
> >  
> > -static void *ctrl_thread_init(void *arg)
> > +static int ctrl_thread_init(void *arg)
> >  {
> >  	struct internal_config *internal_conf =
> >  		eal_get_internal_configuration();
> >  	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
> >  	struct rte_thread_ctrl_params *params = arg;
> > -	void *(*start_routine)(void *) = params->start_routine;
> > -	void *routine_arg = params->arg;
> >  
> >  	__rte_thread_init(rte_lcore_id(), cpuset);
> >  	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
> > @@ -258,13 +259,35 @@ static void *ctrl_thread_init(void *arg)
> >  	if (params->ret != 0) {
> >  		__atomic_store_n(&params->ctrl_thread_status,
> >  			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
> > -		return NULL;
> > +		return params->ret;
> >  	}
> >  
> >  	__atomic_store_n(&params->ctrl_thread_status,
> >  		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
> >  
> > -	return start_routine(routine_arg);
> > +	return 0;
> > +}
> > +
> > +static void *ctrl_thread_start(void *arg)
> > +{
> > +	struct rte_thread_ctrl_params *params = arg;
> > +	void *(*start_routine)(void *) = params->u.start_routine;
> > +
> > +	if (0 != ctrl_thread_init(arg))
> > +		return NULL;
> 
> DPDK uses the Linux not Windows coding style.
> Windows uses the constant on left side of comparison because of the common
> programming error of putting assignment where conditional was intended.
> Linux uses a compiler that puts out a warning, so the more natural
> style is action != result

yes, of course.

i will fix this along with another minor style problem picked up by the
CI.

thanks.
Tyler Retzlaff Dec. 6, 2022, 5:35 p.m. UTC | #3 
On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> On Mon,  5 Dec 2022 12:24:26 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> >  lib/eal/version.map                |  3 ++
> >  3 files changed, 117 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > index c5d8b43..ca85c51 100644
> > --- a/lib/eal/common/eal_common_thread.c
> > +++ b/lib/eal/common/eal_common_thread.c
> > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> >  };
> >  
> >  struct rte_thread_ctrl_params {
> > -	void *(*start_routine)(void *);
> > +	union {
> > +		void * (*start_routine)(void *);
> > +		rte_thread_func thread_func;
> > +	} u;
> 
> Why not just use rte_thread_func, this in internal.

I'm not sure i completely understand your comment here. The main reason
for using a union is to avoid dealing with casting.  Later when the
rte_ctrl_thread_create is deprecated the union will be discarded.

No change was made in v2 but if you still think this should be addressed
let me know.
Stephen Hemminger Dec. 7, 2022, 12:49 a.m. UTC | #4 
On Tue, 6 Dec 2022 09:35:14 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Mon, Dec 05, 2022 at 01:11:16PM -0800, Stephen Hemminger wrote:
> > On Mon,  5 Dec 2022 12:24:26 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >   
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > >  lib/eal/common/eal_common_thread.c | 93 ++++++++++++++++++++++++++++++++++----
> > >  lib/eal/include/rte_thread.h       | 29 ++++++++++++
> > >  lib/eal/version.map                |  3 ++
> > >  3 files changed, 117 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
> > > index c5d8b43..ca85c51 100644
> > > --- a/lib/eal/common/eal_common_thread.c
> > > +++ b/lib/eal/common/eal_common_thread.c
> > > @@ -234,7 +234,10 @@ enum __rte_ctrl_thread_status {
> > >  };
> > >  
> > >  struct rte_thread_ctrl_params {
> > > -	void *(*start_routine)(void *);
> > > +	union {
> > > +		void * (*start_routine)(void *);
> > > +		rte_thread_func thread_func;
> > > +	} u;  
> > 
> > Why not just use rte_thread_func, this in internal.  
> 
> I'm not sure i completely understand your comment here. The main reason
> for using a union is to avoid dealing with casting.  Later when the
> rte_ctrl_thread_create is deprecated the union will be discarded.
> 
> No change was made in v2 but if you still think this should be addressed
> let me know.

Is it possible to do it without union or casting.
Aliasing function pointers is something that leads to long term issues.
The Linux kernel developers are actively fixing up all the casting there;
would like DPDK to adopt the same hygiene.
diff mbox series

Patch

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index c5d8b43..ca85c51 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -234,7 +234,10 @@  enum __rte_ctrl_thread_status {
 };
 
 struct rte_thread_ctrl_params {
-	void *(*start_routine)(void *);
+	union {
+		void * (*start_routine)(void *);
+		rte_thread_func thread_func;
+	} u;
 	void *arg;
 	int ret;
 	/* Control thread status.
@@ -243,14 +246,12 @@  struct rte_thread_ctrl_params {
 	enum __rte_ctrl_thread_status ctrl_thread_status;
 };
 
-static void *ctrl_thread_init(void *arg)
+static int ctrl_thread_init(void *arg)
 {
 	struct internal_config *internal_conf =
 		eal_get_internal_configuration();
 	rte_cpuset_t *cpuset = &internal_conf->ctrl_cpuset;
 	struct rte_thread_ctrl_params *params = arg;
-	void *(*start_routine)(void *) = params->start_routine;
-	void *routine_arg = params->arg;
 
 	__rte_thread_init(rte_lcore_id(), cpuset);
 	params->ret = pthread_setaffinity_np(pthread_self(), sizeof(*cpuset),
@@ -258,13 +259,35 @@  static void *ctrl_thread_init(void *arg)
 	if (params->ret != 0) {
 		__atomic_store_n(&params->ctrl_thread_status,
 			CTRL_THREAD_ERROR, __ATOMIC_RELEASE);
-		return NULL;
+		return params->ret;
 	}
 
 	__atomic_store_n(&params->ctrl_thread_status,
 		CTRL_THREAD_RUNNING, __ATOMIC_RELEASE);
 
-	return start_routine(routine_arg);
+	return 0;
+}
+
+static void *ctrl_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	void *(*start_routine)(void *) = params->u.start_routine;
+
+	if (0 != ctrl_thread_init(arg))
+		return NULL;
+
+	return start_routine(params->arg);
+}
+
+static uint32_t control_thread_start(void *arg)
+{
+	struct rte_thread_ctrl_params *params = arg;
+	rte_thread_func start_routine = params->u.thread_func;
+
+	if (0 != ctrl_thread_init(arg))
+		return params->ret;
+
+	return start_routine(params->arg);
 }
 
 int
@@ -280,12 +303,12 @@  static void *ctrl_thread_init(void *arg)
 	if (!params)
 		return -ENOMEM;
 
-	params->start_routine = start_routine;
+	params->u.start_routine = start_routine;
 	params->arg = arg;
 	params->ret = 0;
 	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
 
-	ret = pthread_create(thread, attr, ctrl_thread_init, (void *)params);
+	ret = pthread_create(thread, attr, ctrl_thread_start, (void *)params);
 	if (ret != 0) {
 		free(params);
 		return -ret;
@@ -322,6 +345,60 @@  static void *ctrl_thread_init(void *arg)
 }
 
 int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+	const rte_thread_attr_t *attr,
+	rte_thread_func start_routine, void *arg)
+{
+	struct rte_thread_ctrl_params *params;
+	enum __rte_ctrl_thread_status ctrl_thread_status;
+	int ret;
+
+	params = malloc(sizeof(*params));
+	if (!params)
+		return -ENOMEM;
+
+	params->u.thread_func = start_routine;
+	params->arg = arg;
+	params->ret = 0;
+	params->ctrl_thread_status = CTRL_THREAD_LAUNCHING;
+
+	ret = rte_thread_create(thread, attr, control_thread_start, params);
+	if (ret != 0) {
+		free(params);
+		return -ret;
+	}
+
+	if (name != NULL) {
+		ret = rte_thread_setname(thread->opaque_id, name);
+		if (ret < 0)
+			RTE_LOG(DEBUG, EAL,
+				"Cannot set name for ctrl thread\n");
+	}
+
+	/* Wait for the control thread to initialize successfully */
+	while ((ctrl_thread_status =
+			__atomic_load_n(&params->ctrl_thread_status,
+			__ATOMIC_ACQUIRE)) == CTRL_THREAD_LAUNCHING) {
+		/* Yield the CPU. Using sched_yield call requires maintaining
+		 * another implementation for Windows as sched_yield is not
+		 * supported on Windows.
+		 */
+		rte_delay_us_sleep(1);
+	}
+
+	/* Check if the control thread encountered an error */
+	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
+		/* ctrl thread is exiting */
+		rte_thread_join(*thread, NULL);
+	}
+
+	ret = params->ret;
+	free(params);
+
+	return ret;
+}
+
+int
 rte_thread_register(void)
 {
 	unsigned int lcore_id;
diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h
index b9edf70..ffc6207 100644
--- a/lib/eal/include/rte_thread.h
+++ b/lib/eal/include/rte_thread.h
@@ -95,6 +95,35 @@  int rte_thread_create(rte_thread_t *thread_id,
 		rte_thread_func thread_func, void *arg);
 
 /**
+ * Create a control thread.
+ *
+ * Creates a control thread with the given name and attributes. The
+ * affinity of the new thread is based on the CPU affinity retrieved
+ * at the time rte_eal_init() was called, the dataplane and service
+ * lcores are then excluded. If setting the name of the thread fails,
+ * the error is ignored and a debug message is logged.
+ *
+ * @param thread
+ *   Filled with the thread id of the new created thread.
+ * @param name
+ *   The name of the control thread (max 16 characters including '\0').
+ * @param attr
+ *   Attributes for the new thread.
+ * @param start_routine
+ *   Function to be executed by the new thread.
+ * @param arg
+ *   Argument passed to start_routine.
+ * @return
+ *   On success, returns 0; on error, it returns a negative value
+ *   corresponding to the error number.
+ */
+__rte_experimental
+int
+rte_control_thread_create(rte_thread_t *thread, const char *name,
+		const rte_thread_attr_t *thread_attr,
+		rte_thread_func thread_func, void *arg);
+
+/**
  * @warning
  * @b EXPERIMENTAL: this API may change without prior notice.
  *
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7ad12a7..8f9eeb9 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -440,6 +440,9 @@  EXPERIMENTAL {
 	rte_thread_detach;
 	rte_thread_equal;
 	rte_thread_join;
+
+	# added in 23.03
+	rte_control_thread_create;
 };
 
 INTERNAL {