[PATCH 0/2] enable lcores test on Windows

[PATCH 0/2] enable lcores test on Windows

[Tyler Retzlaff]

Enable the lcores test on Windows instead of skipping it.

Two bugs are fixed to allow this test to build, run & pass.
  * Mark memory configuration complete during rte_eal_init()
  * Use rte thread api to get a proper implementation of thread join.

Tyler Retzlaff (2):
  eal: add missing call marking memory config complete
  test: enable lcores test on Windows

 app/test/test_lcores.c | 28 +++++++++++++---------------
 lib/eal/windows/eal.c  |  3 +++
 2 files changed, 16 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] eal: add missing call marking memory config complete

[Tyler Retzlaff]

Memory configuration was not being marked as completed add the missing
call to rte_eal_init() for Windows.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/windows/eal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index adb929a..56fadc7 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -462,6 +462,9 @@ enum rte_proc_type_t
 	 */
 	rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MAIN);
 	rte_eal_mp_wait_lcore();
+
+	eal_mcfg_complete();
+
 	return fctret;
 }
 
-- 
1.8.3.1

[PATCH 2/2] test: enable lcores test on Windows

[Tyler Retzlaff]

Stop using pthread and convert the test to use EAL thread APIs. Because
the EAL thread APIs provide more than just a stub for thread join on
Windows the tests now pass and need not be skipped.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..5b43aa5 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,17 +8,18 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
 struct thread_context {
 	enum { Thread_INIT, Thread_ERROR, Thread_DONE } state;
 	bool lcore_id_any;
-	pthread_t id;
+	rte_thread_t id;
 	unsigned int *registered_count;
 };
 
-static void *thread_loop(void *arg)
+static uint32_t thread_loop(void *arg)
 {
 	struct thread_context *t = arg;
 	unsigned int lcore_id;
@@ -55,7 +56,7 @@ static void *thread_loop(void *arg)
 	if (t->state != Thread_ERROR)
 		t->state = Thread_DONE;
 
-	return NULL;
+	return 0;
 }
 
 static int
@@ -77,7 +78,7 @@ static void *thread_loop(void *arg)
 		t->state = Thread_INIT;
 		t->registered_count = &registered_count;
 		t->lcore_id_any = false;
-		if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+		if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 			break;
 		non_eal_threads_count++;
 	}
@@ -96,7 +97,7 @@ static void *thread_loop(void *arg)
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = true;
-	if (pthread_create(&t->id, NULL, thread_loop, t) == 0) {
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) == 0) {
 		non_eal_threads_count++;
 		printf("non-EAL threads count: %u\n", non_eal_threads_count);
 		while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -110,7 +111,7 @@ static void *thread_loop(void *arg)
 	ret = 0;
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 		if (t->state != Thread_DONE)
 			ret = -1;
 	}
@@ -262,7 +263,7 @@ struct limit_lcore_context {
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = false;
-	if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 		goto cleanup_threads;
 	non_eal_threads_count++;
 	while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -285,7 +286,7 @@ struct limit_lcore_context {
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = true;
-	if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 		goto cleanup_threads;
 	non_eal_threads_count++;
 	while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -309,7 +310,7 @@ struct limit_lcore_context {
 	ret = 0;
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 		if (t->state != Thread_DONE)
 			ret = -1;
 	}
@@ -330,7 +331,7 @@ struct limit_lcore_context {
 	__atomic_store_n(&registered_count, 0, __ATOMIC_RELEASE);
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 	}
 error:
 	if (handle[1] != NULL)
@@ -361,7 +362,7 @@ static void *ctrl_thread_loop(void *arg)
 	/* Create one control thread */
 	t = &ctrl_thread_context;
 	t->state = Thread_INIT;
-	if (rte_ctrl_thread_create(&t->id, "test_ctrl_threads",
+	if (rte_ctrl_thread_create((pthread_t *)&t->id, "test_ctrl_threads",
 					NULL, ctrl_thread_loop, t) != 0)
 		return -1;
 
@@ -369,7 +370,7 @@ static void *ctrl_thread_loop(void *arg)
 	 * This also acts as the barrier such that the memory operations
 	 * in control thread are visible to this thread.
 	 */
-	pthread_join(t->id, NULL);
+	rte_thread_join(t->id, NULL);
 
 	/* Check if the control thread set the correct state */
 	if (t->state != Thread_DONE)
@@ -384,9 +385,6 @@ static void *ctrl_thread_loop(void *arg)
 	unsigned int eal_threads_count = 0;
 	unsigned int i;
 
-	if (RTE_EXEC_ENV_IS_WINDOWS)
-		return TEST_SKIPPED;
-
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
 		if (!rte_lcore_has_role(i, ROLE_OFF))
 			eal_threads_count++;
-- 
1.8.3.1

Re: [PATCH 0/2] enable lcores test on Windows

[Tyler Retzlaff]

hey folks,

a quick ping looking for review here. simple fix and increases test
coverage. fairly straight forward.

thanks!

On Mon, Dec 05, 2022 at 04:39:27PM -0800, Tyler Retzlaff wrote:
> Enable the lcores test on Windows instead of skipping it.
> 
> Two bugs are fixed to allow this test to build, run & pass.
>   * Mark memory configuration complete during rte_eal_init()
>   * Use rte thread api to get a proper implementation of thread join.
> 
> Tyler Retzlaff (2):
>   eal: add missing call marking memory config complete
>   test: enable lcores test on Windows
> 
>  app/test/test_lcores.c | 28 +++++++++++++---------------
>  lib/eal/windows/eal.c  |  3 +++
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH 1/2] eal: add missing call marking memory config complete

[Stephen Hemminger]

On Mon,  5 Dec 2022 16:39:28 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Memory configuration was not being marked as completed add the missing
> call to rte_eal_init() for Windows.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  lib/eal/windows/eal.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
> index adb929a..56fadc7 100644
> --- a/lib/eal/windows/eal.c
> +++ b/lib/eal/windows/eal.c
> @@ -462,6 +462,9 @@ enum rte_proc_type_t
>  	 */
>  	rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MAIN);
>  	rte_eal_mp_wait_lcore();
> +
> +	eal_mcfg_complete();
> +
>  	return fctret;
>  }
>  

Should this have a Fixes tag?
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 2/2] test: enable lcores test on Windows

[Stephen Hemminger]

On Mon,  5 Dec 2022 16:39:29 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> Stop using pthread and convert the test to use EAL thread APIs. Because
> the EAL thread APIs provide more than just a stub for thread join on
> Windows the tests now pass and need not be skipped.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

This makes sense to cover the API's even if not on Windows.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 1/2] eal: add missing call marking memory config complete

[David Marchand]

On Tue, Dec 6, 2022 at 1:39 AM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Memory configuration was not being marked as completed add the missing
> call to rte_eal_init() for Windows.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

The change is ok.

The commitlog misses some context: I guess the point is to let
rte_thread_register work on Windows.
So we can mark this change as:
Fixes: 5c307ba2a5b1 ("eal: register non-EAL threads as lcores")

WDYT?


In any case, this change won't fix anything related to mp.
If mp is supported one day on Windows, other calls to eal_mcfg* would
be required.


-- 
David Marchand

Re: [PATCH 1/2] eal: add missing call marking memory config complete

[Tyler Retzlaff]

On Thu, Dec 15, 2022 at 10:59:07AM +0100, David Marchand wrote:
> On Tue, Dec 6, 2022 at 1:39 AM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Memory configuration was not being marked as completed add the missing
> > call to rte_eal_init() for Windows.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> The change is ok.
> 
> The commitlog misses some context: I guess the point is to let
> rte_thread_register work on Windows.

oh i guess i better provide a better message. the reason is i want to
run the test_ctrl_thread() (in test_lcores.c) which is an api available
on windows. maybe when i add the replacement control thread creation it
should be moved to / live in test_threads.c?

it is nice as a side-effect it did pick up that the mcfg hadn't been
marked as done. (though i understand that's not the purpose of the
test).

i'll have another whack at changing the log.

> So we can mark this change as:
> Fixes: 5c307ba2a5b1 ("eal: register non-EAL threads as lcores")
> 
> WDYT?

yep, v2 will fix the message & the Fixes tag.

> 
> 
> In any case, this change won't fix anything related to mp.
> If mp is supported one day on Windows, other calls to eal_mcfg* would
> be required.

agreed.

> 
> -- 
> David Marchand

[PATCH v2 0/2] Enable the lcores test on Windows instead of skipping it.

[Tyler Retzlaff]

Two bugs are fixed to allow this test to build, run & pass.
  * Mark memory configuration complete during rte_eal_init()
  * Use rte thread api to get a proper implementation of thread join.

v2:
  * update commit message to clarify why this test is beneficial on Windows.
  * add missing Fixes tag

Tyler Retzlaff (2):
  eal: add missing call marking memory config complete
  test: enable lcores test on Windows

 app/test/test_lcores.c | 28 +++++++++++++---------------
 lib/eal/windows/eal.c  |  3 +++
 2 files changed, 16 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] eal: add missing call marking memory config complete

[Tyler Retzlaff]

Memory configuration was not being marked as completed add the missing
call to rte_eal_init() for Windows.

Allows rte_thread_register to work on Windows and lcores_autotest to be
built and run Windows which also exercises the rte_ctrl_thread_create
API on Windows.

Fixes: 5c307ba2a5b1 ("eal: register non-EAL threads as lcores")

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/windows/eal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index adb929a..56fadc7 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -462,6 +462,9 @@ enum rte_proc_type_t
 	 */
 	rte_eal_mp_remote_launch(sync_func, NULL, SKIP_MAIN);
 	rte_eal_mp_wait_lcore();
+
+	eal_mcfg_complete();
+
 	return fctret;
 }
 
-- 
1.8.3.1

[PATCH v2 2/2] test: enable lcores test on Windows

[Tyler Retzlaff]

Stop using pthread and convert the test to use EAL thread APIs. Because
the EAL thread APIs provide more than just a stub for thread join on
Windows the tests now pass and need not be skipped.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 app/test/test_lcores.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c
index a6bb412..5b43aa5 100644
--- a/app/test/test_lcores.c
+++ b/app/test/test_lcores.c
@@ -8,17 +8,18 @@
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_lcore.h>
+#include <rte_thread.h>
 
 #include "test.h"
 
 struct thread_context {
 	enum { Thread_INIT, Thread_ERROR, Thread_DONE } state;
 	bool lcore_id_any;
-	pthread_t id;
+	rte_thread_t id;
 	unsigned int *registered_count;
 };
 
-static void *thread_loop(void *arg)
+static uint32_t thread_loop(void *arg)
 {
 	struct thread_context *t = arg;
 	unsigned int lcore_id;
@@ -55,7 +56,7 @@ static void *thread_loop(void *arg)
 	if (t->state != Thread_ERROR)
 		t->state = Thread_DONE;
 
-	return NULL;
+	return 0;
 }
 
 static int
@@ -77,7 +78,7 @@ static void *thread_loop(void *arg)
 		t->state = Thread_INIT;
 		t->registered_count = &registered_count;
 		t->lcore_id_any = false;
-		if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+		if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 			break;
 		non_eal_threads_count++;
 	}
@@ -96,7 +97,7 @@ static void *thread_loop(void *arg)
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = true;
-	if (pthread_create(&t->id, NULL, thread_loop, t) == 0) {
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) == 0) {
 		non_eal_threads_count++;
 		printf("non-EAL threads count: %u\n", non_eal_threads_count);
 		while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -110,7 +111,7 @@ static void *thread_loop(void *arg)
 	ret = 0;
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 		if (t->state != Thread_DONE)
 			ret = -1;
 	}
@@ -262,7 +263,7 @@ struct limit_lcore_context {
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = false;
-	if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 		goto cleanup_threads;
 	non_eal_threads_count++;
 	while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -285,7 +286,7 @@ struct limit_lcore_context {
 	t->state = Thread_INIT;
 	t->registered_count = &registered_count;
 	t->lcore_id_any = true;
-	if (pthread_create(&t->id, NULL, thread_loop, t) != 0)
+	if (rte_thread_create(&t->id, NULL, thread_loop, t) != 0)
 		goto cleanup_threads;
 	non_eal_threads_count++;
 	while (__atomic_load_n(&registered_count, __ATOMIC_ACQUIRE) !=
@@ -309,7 +310,7 @@ struct limit_lcore_context {
 	ret = 0;
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 		if (t->state != Thread_DONE)
 			ret = -1;
 	}
@@ -330,7 +331,7 @@ struct limit_lcore_context {
 	__atomic_store_n(&registered_count, 0, __ATOMIC_RELEASE);
 	for (i = 0; i < non_eal_threads_count; i++) {
 		t = &thread_contexts[i];
-		pthread_join(t->id, NULL);
+		rte_thread_join(t->id, NULL);
 	}
 error:
 	if (handle[1] != NULL)
@@ -361,7 +362,7 @@ static void *ctrl_thread_loop(void *arg)
 	/* Create one control thread */
 	t = &ctrl_thread_context;
 	t->state = Thread_INIT;
-	if (rte_ctrl_thread_create(&t->id, "test_ctrl_threads",
+	if (rte_ctrl_thread_create((pthread_t *)&t->id, "test_ctrl_threads",
 					NULL, ctrl_thread_loop, t) != 0)
 		return -1;
 
@@ -369,7 +370,7 @@ static void *ctrl_thread_loop(void *arg)
 	 * This also acts as the barrier such that the memory operations
 	 * in control thread are visible to this thread.
 	 */
-	pthread_join(t->id, NULL);
+	rte_thread_join(t->id, NULL);
 
 	/* Check if the control thread set the correct state */
 	if (t->state != Thread_DONE)
@@ -384,9 +385,6 @@ static void *ctrl_thread_loop(void *arg)
 	unsigned int eal_threads_count = 0;
 	unsigned int i;
 
-	if (RTE_EXEC_ENV_IS_WINDOWS)
-		return TEST_SKIPPED;
-
 	for (i = 0; i < RTE_MAX_LCORE; i++) {
 		if (!rte_lcore_has_role(i, ROLE_OFF))
 			eal_threads_count++;
-- 
1.8.3.1

Re: [PATCH v2 0/2] Enable the lcores test on Windows instead of skipping it.

[David Marchand]

On Fri, Dec 16, 2022 at 6:16 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Two bugs are fixed to allow this test to build, run & pass.
>   * Mark memory configuration complete during rte_eal_init()
>   * Use rte thread api to get a proper implementation of thread join.
>
> v2:
>   * update commit message to clarify why this test is beneficial on Windows.
>   * add missing Fixes tag
>
> Tyler Retzlaff (2):
>   eal: add missing call marking memory config complete
>   test: enable lcores test on Windows
>
>  app/test/test_lcores.c | 28 +++++++++++++---------------
>  lib/eal/windows/eal.c  |  3 +++
>  2 files changed, 16 insertions(+), 15 deletions(-)

For the series,
Acked-by: David Marchand <david.marchand@redhat.com>

I added review tags from Stephen from v1.
Series applied, thanks.


-- 
David Marchand