[PATCH 0/6] windows: remove most pthread lifetime shim functions

[PATCH 0/6] windows: remove most pthread lifetime shim functions

[Tyler Retzlaff]

Adopt rte thread APIs in code built for Windows to decouple it from the
pthread shim.

Remove most of the pthread_xxx lifetime shim functions, only
pthread_create remains while we wait for rte_ctrl_thread_create removal.

Tyler Retzlaff (6):
  dma/skeleton: use rte thread API
  net/ixgbe: use rte thread API
  net/ice: use rte thread API
  net/iavf: use rte thread API
  eal: use rte thread API
  windows: remove most pthread lifetime shim functions

 drivers/dma/skeleton/skeleton_dmadev.c | 15 +++---
 drivers/dma/skeleton/skeleton_dmadev.h |  4 +-
 drivers/net/iavf/iavf_vchnl.c          | 12 ++---
 drivers/net/ice/ice_dcf_parent.c       | 11 ++--
 drivers/net/ixgbe/ixgbe_ethdev.c       | 10 ++--
 drivers/net/ixgbe/ixgbe_ethdev.h       |  2 +-
 lib/eal/common/eal_common_thread.c     |  4 +-
 lib/eal/windows/eal.c                  |  2 +-
 lib/eal/windows/eal_interrupts.c       | 12 ++---
 lib/eal/windows/include/pthread.h      | 99 ----------------------------------
 10 files changed, 36 insertions(+), 135 deletions(-)

-- 
1.8.3.1

[PATCH 1/6] dma/skeleton: use rte thread API

[Tyler Retzlaff]

Update driver to use rte thread API where available instead of pthread
as a prerequisite to removing pthread stubs on Windows.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/dma/skeleton/skeleton_dmadev.c | 15 ++++++++-------
 drivers/dma/skeleton/skeleton_dmadev.h |  4 ++--
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/skeleton/skeleton_dmadev.c b/drivers/dma/skeleton/skeleton_dmadev.c
index daf35ec..2ec10db 100644
--- a/drivers/dma/skeleton/skeleton_dmadev.c
+++ b/drivers/dma/skeleton/skeleton_dmadev.c
@@ -5,6 +5,8 @@
 #include <inttypes.h>
 #include <stdlib.h>
 
+#include <pthread.h>
+
 #include <bus_vdev_driver.h>
 #include <rte_cycles.h>
 #include <rte_eal.h>
@@ -53,7 +55,7 @@
 	return 0;
 }
 
-static void *
+static uint32_t
 cpucopy_thread(void *param)
 {
 #define SLEEP_THRESHOLD		10000
@@ -81,7 +83,7 @@
 		(void)rte_ring_enqueue(hw->desc_completed, (void *)desc);
 	}
 
-	return NULL;
+	return 0;
 }
 
 static void
@@ -126,7 +128,7 @@
 	rte_mb();
 
 	snprintf(name, sizeof(name), "dma_skel_%d", dev->data->dev_id);
-	ret = rte_ctrl_thread_create(&hw->thread, name, NULL,
+	ret = rte_thread_create_control(&hw->thread, name, NULL,
 				     cpucopy_thread, dev);
 	if (ret) {
 		SKELDMA_LOG(ERR, "Start cpucopy thread fail!");
@@ -135,8 +137,7 @@
 
 	if (hw->lcore_id != -1) {
 		cpuset = rte_lcore_cpuset(hw->lcore_id);
-		ret = pthread_setaffinity_np(hw->thread, sizeof(cpuset),
-					     &cpuset);
+		ret = rte_thread_get_affinity_by_id(hw->thread, &cpuset);
 		if (ret)
 			SKELDMA_LOG(WARNING,
 				"Set thread affinity lcore = %d fail!",
@@ -154,8 +155,8 @@
 	hw->exit_flag = true;
 	rte_delay_ms(1);
 
-	(void)pthread_cancel(hw->thread);
-	pthread_join(hw->thread, NULL);
+	(void)pthread_cancel((pthread_t)hw->thread.opaque_id);
+	rte_thread_join(hw->thread, NULL);
 
 	return 0;
 }
diff --git a/drivers/dma/skeleton/skeleton_dmadev.h b/drivers/dma/skeleton/skeleton_dmadev.h
index 6f89400..8670a68 100644
--- a/drivers/dma/skeleton/skeleton_dmadev.h
+++ b/drivers/dma/skeleton/skeleton_dmadev.h
@@ -5,9 +5,9 @@
 #ifndef SKELETON_DMADEV_H
 #define SKELETON_DMADEV_H
 
-#include <pthread.h>
 
 #include <rte_ring.h>
+#include <rte_thread.h>
 
 #define SKELDMA_ARG_LCORE	"lcore"
 
@@ -21,7 +21,7 @@ struct skeldma_desc {
 struct skeldma_hw {
 	int lcore_id; /* cpucopy task affinity core */
 	int socket_id;
-	pthread_t thread; /* cpucopy task thread */
+	rte_thread_t thread; /* cpucopy task thread */
 	volatile int exit_flag; /* cpucopy task exit flag */
 
 	struct skeldma_desc *desc_mem;
-- 
1.8.3.1

[PATCH 2/6] net/ixgbe: use rte thread API

[Tyler Retzlaff]

Update driver to use rte thread API where available instead of pthread
as a prerequisite to removing pthread stubs on Windows.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 10 +++++-----
 drivers/net/ixgbe/ixgbe_ethdev.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 88118bc..3abe96e 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -236,7 +236,7 @@ static int ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev,
 static int ixgbe_dev_interrupt_action(struct rte_eth_dev *dev);
 static void ixgbe_dev_interrupt_handler(void *param);
 static void ixgbe_dev_interrupt_delayed_handler(void *param);
-static void *ixgbe_dev_setup_link_thread_handler(void *param);
+static uint32_t ixgbe_dev_setup_link_thread_handler(void *param);
 static int ixgbe_dev_wait_setup_link_complete(struct rte_eth_dev *dev,
 					      uint32_t timeout_ms);
 
@@ -4203,7 +4203,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	return 1;
 }
 
-static void *
+static uint32_t
 ixgbe_dev_setup_link_thread_handler(void *param)
 {
 	struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
@@ -4214,7 +4214,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 	u32 speed;
 	bool autoneg = false;
 
-	pthread_detach(pthread_self());
+	rte_thread_detach(rte_thread_self());
 	speed = hw->phy.autoneg_advertised;
 	if (!speed)
 		ixgbe_get_link_capabilities(hw, &speed, &autoneg);
@@ -4223,7 +4223,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 
 	intr->flags &= ~IXGBE_FLAG_NEED_LINK_CONFIG;
 	rte_atomic32_clear(&ad->link_thread_running);
-	return NULL;
+	return 0;
 }
 
 /*
@@ -4323,7 +4323,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 				 * when there is no link thread running.
 				 */
 				intr->flags |= IXGBE_FLAG_NEED_LINK_CONFIG;
-				if (rte_ctrl_thread_create(&ad->link_thread_tid,
+				if (rte_thread_create_control(&ad->link_thread_tid,
 					"ixgbe-link-handler",
 					NULL,
 					ixgbe_dev_setup_link_thread_handler,
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 48290af..4332b2c 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -511,7 +511,7 @@ struct ixgbe_adapter {
 	uint8_t pflink_fullchk;
 	uint8_t mac_ctrl_frame_fwd;
 	rte_atomic32_t link_thread_running;
-	pthread_t link_thread_tid;
+	rte_thread_t link_thread_tid;
 };
 
 struct ixgbe_vf_representor {
-- 
1.8.3.1

[PATCH 3/6] net/ice: use rte thread API

[Tyler Retzlaff]

Update driver to use rte thread API where available instead of pthread
as a prerequisite to removing pthread stubs on Windows.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/ice/ice_dcf_parent.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ice/ice_dcf_parent.c b/drivers/net/ice/ice_dcf_parent.c
index 01e390d..3175d18 100644
--- a/drivers/net/ice/ice_dcf_parent.c
+++ b/drivers/net/ice/ice_dcf_parent.c
@@ -4,7 +4,6 @@
 #include <stdlib.h>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <pthread.h>
 #include <unistd.h>
 
 #include <rte_spinlock.h>
@@ -115,7 +114,7 @@ struct ice_dcf_reset_event_param {
 			pf_vsi_idx, vsi_ctx->vsi_num);
 }
 
-static void*
+static uint32_t
 ice_dcf_vsi_update_service_handler(void *param)
 {
 	struct ice_dcf_reset_event_param *reset_param = param;
@@ -124,7 +123,7 @@ struct ice_dcf_reset_event_param {
 		container_of(hw, struct ice_dcf_adapter, real_hw);
 	struct ice_adapter *parent_adapter = &adapter->parent;
 
-	pthread_detach(pthread_self());
+	rte_thread_detach(rte_thread_self());
 
 	rte_delay_us(ICE_DCF_VSI_UPDATE_SERVICE_INTERVAL);
 
@@ -154,7 +153,7 @@ struct ice_dcf_reset_event_param {
 
 	free(param);
 
-	return NULL;
+	return 0;
 }
 
 static void
@@ -163,7 +162,7 @@ struct ice_dcf_reset_event_param {
 #define THREAD_NAME_LEN	16
 	struct ice_dcf_reset_event_param *param;
 	char name[THREAD_NAME_LEN];
-	pthread_t thread;
+	rte_thread_t thread;
 	int ret;
 
 	param = malloc(sizeof(*param));
@@ -177,7 +176,7 @@ struct ice_dcf_reset_event_param {
 	param->vf_id = vf_id;
 
 	snprintf(name, sizeof(name), "ice-reset-%u", vf_id);
-	ret = rte_ctrl_thread_create(&thread, name, NULL,
+	ret = rte_thread_create_control(&thread, name, NULL,
 				     ice_dcf_vsi_update_service_handler, param);
 	if (ret != 0) {
 		PMD_DRV_LOG(ERR, "Failed to start the thread for reset handling");
-- 
1.8.3.1

[PATCH 4/6] net/iavf: use rte thread API

[Tyler Retzlaff]

Update driver to use rte thread API where available instead of pthread
as a prerequisite to removing pthread stubs on Windows.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/iavf/iavf_vchnl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 9adaadb..7a2be22 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -42,7 +42,7 @@ struct iavf_event_element {
 
 struct iavf_event_handler {
 	uint32_t ndev;
-	pthread_t tid;
+	rte_thread_t tid;
 	int fd[2];
 	pthread_mutex_t lock;
 	TAILQ_HEAD(event_list, iavf_event_element) pending;
@@ -59,7 +59,7 @@ struct iavf_event_handler {
 	(var) = (tvar))
 #endif
 
-static void *
+static uint32_t
 iavf_dev_event_handle(void *param __rte_unused)
 {
 	struct iavf_event_handler *handler = &event_handler;
@@ -84,7 +84,7 @@ struct iavf_event_handler {
 		}
 	}
 
-	return NULL;
+	return 0;
 }
 
 static void
@@ -135,7 +135,7 @@ struct iavf_event_handler {
 	TAILQ_INIT(&handler->pending);
 	pthread_mutex_init(&handler->lock, NULL);
 
-	if (rte_ctrl_thread_create(&handler->tid, "iavf-event-thread",
+	if (rte_thread_create_control(&handler->tid, "iavf-event-thread",
 				NULL, iavf_dev_event_handle, NULL)) {
 		__atomic_sub_fetch(&handler->ndev, 1, __ATOMIC_RELAXED);
 		return -1;
@@ -152,14 +152,14 @@ struct iavf_event_handler {
 	if (__atomic_sub_fetch(&handler->ndev, 1, __ATOMIC_RELAXED) != 0)
 		return;
 
-	int unused = pthread_cancel(handler->tid);
+	int unused = pthread_cancel((pthread_t)handler->tid.opaque_id);
 	RTE_SET_USED(unused);
 	close(handler->fd[0]);
 	close(handler->fd[1]);
 	handler->fd[0] = -1;
 	handler->fd[1] = -1;
 
-	pthread_join(handler->tid, NULL);
+	rte_thread_join(handler->tid, NULL);
 	pthread_mutex_destroy(&handler->lock);
 
 	struct iavf_event_element *pos, *save_next;
-- 
1.8.3.1

[PATCH 5/6] eal: use rte thread API

[Tyler Retzlaff]

Update driver to use rte thread API where available instead of pthread
as a prerequisite to removing pthread stubs on Windows.

There is a single pthread_create still in use until
rte_ctrl_thread_create is removed.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/common/eal_common_thread.c |  4 ++--
 lib/eal/windows/eal.c              |  2 +-
 lib/eal/windows/eal_interrupts.c   | 12 ++++++------
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/eal/common/eal_common_thread.c b/lib/eal/common/eal_common_thread.c
index 079a385..e3aad2c 100644
--- a/lib/eal/common/eal_common_thread.c
+++ b/lib/eal/common/eal_common_thread.c
@@ -176,7 +176,7 @@ unsigned rte_socket_id(void)
 
 	ret = eal_thread_dump_current_affinity(cpuset, sizeof(cpuset));
 	RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%zx;cpuset=[%s%s])\n",
-		lcore_id, (uintptr_t)pthread_self(), cpuset,
+		lcore_id, rte_thread_self().opaque_id, cpuset,
 		ret == 0 ? "" : "...");
 
 	rte_eal_trace_thread_lcore_ready(lcore_id, cpuset);
@@ -330,7 +330,7 @@ static uint32_t control_thread_start(void *arg)
 	/* Check if the control thread encountered an error */
 	if (ctrl_thread_status == CTRL_THREAD_ERROR) {
 		/* ctrl thread is exiting */
-		pthread_join(*thread, NULL);
+		rte_thread_join((rte_thread_t){(uintptr_t)*thread}, NULL);
 	}
 
 	ret = params->ret;
diff --git a/lib/eal/windows/eal.c b/lib/eal/windows/eal.c
index e7d405b..89da0c0 100644
--- a/lib/eal/windows/eal.c
+++ b/lib/eal/windows/eal.c
@@ -416,7 +416,7 @@ enum rte_proc_type_t
 
 	ret = eal_thread_dump_current_affinity(cpuset, sizeof(cpuset));
 	RTE_LOG(DEBUG, EAL, "Main lcore %u is ready (tid=%zx;cpuset=[%s%s])\n",
-		config->main_lcore, (uintptr_t)pthread_self(), cpuset,
+		config->main_lcore, rte_thread_self().opaque_id, cpuset,
 		ret == 0 ? "" : "...");
 
 	RTE_LCORE_FOREACH_WORKER(i) {
diff --git a/lib/eal/windows/eal_interrupts.c b/lib/eal/windows/eal_interrupts.c
index bb0585c..49c4b96 100644
--- a/lib/eal/windows/eal_interrupts.c
+++ b/lib/eal/windows/eal_interrupts.c
@@ -9,7 +9,7 @@
 
 #define IOCP_KEY_SHUTDOWN UINT32_MAX
 
-static pthread_t intr_thread;
+static rte_thread_t intr_thread;
 
 static HANDLE intr_iocp;
 static HANDLE intr_thread_handle;
@@ -33,7 +33,7 @@
 	return 0;
 }
 
-static void *
+static uint32_t
 eal_intr_thread_main(LPVOID arg __rte_unused)
 {
 	bool finished = false;
@@ -78,12 +78,12 @@
 	intr_thread_handle = NULL;
 
 cleanup:
-	intr_thread = 0;
+	intr_thread.opaque_id = 0;
 
 	CloseHandle(intr_iocp);
 	intr_iocp = NULL;
 
-	return NULL;
+	return 0;
 }
 
 int
@@ -98,7 +98,7 @@
 		return -1;
 	}
 
-	ret = rte_ctrl_thread_create(&intr_thread, "eal-intr-thread", NULL,
+	ret = rte_thread_create_control(&intr_thread, "eal-intr-thread", NULL,
 			eal_intr_thread_main, NULL);
 	if (ret != 0) {
 		rte_errno = -ret;
@@ -111,7 +111,7 @@
 int
 rte_thread_is_intr(void)
 {
-	return pthread_equal(intr_thread, pthread_self());
+	return rte_thread_equal(intr_thread, rte_thread_self());
 }
 
 int
-- 
1.8.3.1

[PATCH 6/6] windows: remove most pthread lifetime shim functions

[Tyler Retzlaff]

Remove most of the pthread_xxx lifetime shim functions, only
pthread_create remains while we wait for rte_ctrl_thread_create removal.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/windows/include/pthread.h | 99 ---------------------------------------
 1 file changed, 99 deletions(-)

diff --git a/lib/eal/windows/include/pthread.h b/lib/eal/windows/include/pthread.h
index f7cf0e9..051b931 100644
--- a/lib/eal/windows/include/pthread.h
+++ b/lib/eal/windows/include/pthread.h
@@ -42,92 +42,6 @@
 	!DeleteSynchronizationBarrier(barrier)
 #define pthread_cancel(thread) !TerminateThread((HANDLE) thread, 0)
 
-/* pthread function overrides */
-#define pthread_self() \
-	((pthread_t)GetCurrentThreadId())
-
-
-static inline int
-pthread_equal(pthread_t t1, pthread_t t2)
-{
-	return t1 == t2;
-}
-
-static inline int
-pthread_setaffinity_np(pthread_t threadid, size_t cpuset_size,
-			rte_cpuset_t *cpuset)
-{
-	DWORD_PTR ret = 0;
-	HANDLE thread_handle;
-
-	if (cpuset == NULL || cpuset_size == 0)
-		return -1;
-
-	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
-	if (thread_handle == NULL) {
-		RTE_LOG_WIN32_ERR("OpenThread()");
-		return -1;
-	}
-
-	ret = SetThreadAffinityMask(thread_handle, *cpuset->_bits);
-	if (ret == 0) {
-		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
-		goto close_handle;
-	}
-
-close_handle:
-	if (CloseHandle(thread_handle) == 0) {
-		RTE_LOG_WIN32_ERR("CloseHandle()");
-		return -1;
-	}
-	return (ret == 0) ? -1 : 0;
-}
-
-static inline int
-pthread_getaffinity_np(pthread_t threadid, size_t cpuset_size,
-			rte_cpuset_t *cpuset)
-{
-	/* Workaround for the lack of a GetThreadAffinityMask()
-	 *API in Windows
-	 */
-	DWORD_PTR prev_affinity_mask;
-	HANDLE thread_handle;
-	DWORD_PTR ret = 0;
-
-	if (cpuset == NULL || cpuset_size == 0)
-		return -1;
-
-	thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
-	if (thread_handle == NULL) {
-		RTE_LOG_WIN32_ERR("OpenThread()");
-		return -1;
-	}
-
-	/* obtain previous mask by setting dummy mask */
-	prev_affinity_mask = SetThreadAffinityMask(thread_handle, 0x1);
-	if (prev_affinity_mask == 0) {
-		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
-		goto close_handle;
-	}
-
-	/* set it back! */
-	ret = SetThreadAffinityMask(thread_handle, prev_affinity_mask);
-	if (ret == 0) {
-		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
-		goto close_handle;
-	}
-
-	memset(cpuset, 0, cpuset_size);
-	*cpuset->_bits = prev_affinity_mask;
-
-close_handle:
-	if (CloseHandle(thread_handle) == 0) {
-		RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
-		return -1;
-	}
-	return (ret == 0) ? -1 : 0;
-}
-
 static inline int
 pthread_create(void *threadid, const void *threadattr, void *threadfunc,
 		void *args)
@@ -145,19 +59,6 @@
 }
 
 static inline int
-pthread_detach(__rte_unused pthread_t thread)
-{
-	return 0;
-}
-
-static inline int
-pthread_join(__rte_unused pthread_t thread,
-	__rte_unused void **value_ptr)
-{
-	return 0;
-}
-
-static inline int
 pthread_mutex_init(pthread_mutex_t *mutex,
 		   __rte_unused pthread_mutexattr_t *attr)
 {
-- 
1.8.3.1

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[Tyler Retzlaff]

early review if possible please, would like to have this in from the
start of 23.07 to work against.

thanks!

On Fri, Mar 17, 2023 at 03:34:14PM -0700, Tyler Retzlaff wrote:
> Adopt rte thread APIs in code built for Windows to decouple it from the
> pthread shim.
> 
> Remove most of the pthread_xxx lifetime shim functions, only
> pthread_create remains while we wait for rte_ctrl_thread_create removal.
> 
> Tyler Retzlaff (6):
>   dma/skeleton: use rte thread API
>   net/ixgbe: use rte thread API
>   net/ice: use rte thread API
>   net/iavf: use rte thread API
>   eal: use rte thread API
>   windows: remove most pthread lifetime shim functions
> 
>  drivers/dma/skeleton/skeleton_dmadev.c | 15 +++---
>  drivers/dma/skeleton/skeleton_dmadev.h |  4 +-
>  drivers/net/iavf/iavf_vchnl.c          | 12 ++---
>  drivers/net/ice/ice_dcf_parent.c       | 11 ++--
>  drivers/net/ixgbe/ixgbe_ethdev.c       | 10 ++--
>  drivers/net/ixgbe/ixgbe_ethdev.h       |  2 +-
>  lib/eal/common/eal_common_thread.c     |  4 +-
>  lib/eal/windows/eal.c                  |  2 +-
>  lib/eal/windows/eal_interrupts.c       | 12 ++---
>  lib/eal/windows/include/pthread.h      | 99 ----------------------------------
>  10 files changed, 36 insertions(+), 135 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[Tyler Retzlaff]

Hi folks,

just soliciting some review for this orphan of a series. Bruce, Morten
have you got some cycles to take a look?

Thanks

On Fri, Mar 17, 2023 at 03:34:14PM -0700, Tyler Retzlaff wrote:
> Adopt rte thread APIs in code built for Windows to decouple it from the
> pthread shim.
> 
> Remove most of the pthread_xxx lifetime shim functions, only
> pthread_create remains while we wait for rte_ctrl_thread_create removal.
> 
> Tyler Retzlaff (6):
>   dma/skeleton: use rte thread API
>   net/ixgbe: use rte thread API
>   net/ice: use rte thread API
>   net/iavf: use rte thread API
>   eal: use rte thread API
>   windows: remove most pthread lifetime shim functions
> 
>  drivers/dma/skeleton/skeleton_dmadev.c | 15 +++---
>  drivers/dma/skeleton/skeleton_dmadev.h |  4 +-
>  drivers/net/iavf/iavf_vchnl.c          | 12 ++---
>  drivers/net/ice/ice_dcf_parent.c       | 11 ++--
>  drivers/net/ixgbe/ixgbe_ethdev.c       | 10 ++--
>  drivers/net/ixgbe/ixgbe_ethdev.h       |  2 +-
>  lib/eal/common/eal_common_thread.c     |  4 +-
>  lib/eal/windows/eal.c                  |  2 +-
>  lib/eal/windows/eal_interrupts.c       | 12 ++---
>  lib/eal/windows/include/pthread.h      | 99 ----------------------------------
>  10 files changed, 36 insertions(+), 135 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [PATCH 1/6] dma/skeleton: use rte thread API

[Bruce Richardson]

On Fri, Mar 17, 2023 at 03:34:15PM -0700, Tyler Retzlaff wrote:
> Update driver to use rte thread API where available instead of pthread
> as a prerequisite to removing pthread stubs on Windows.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
>  drivers/dma/skeleton/skeleton_dmadev.c | 15 ++++++++-------
>  drivers/dma/skeleton/skeleton_dmadev.h |  4 ++--
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/skeleton/skeleton_dmadev.c b/drivers/dma/skeleton/skeleton_dmadev.c
> index daf35ec..2ec10db 100644
> --- a/drivers/dma/skeleton/skeleton_dmadev.c
> +++ b/drivers/dma/skeleton/skeleton_dmadev.c
> @@ -5,6 +5,8 @@
>  #include <inttypes.h>
>  #include <stdlib.h>
>  
> +#include <pthread.h>
> +

Curious as to why this is needed here. Does rte_thread.h not include all
needed threading dependencies?
[Self-answer] I assume this is for pthread_cancel below, right?


>  #include <bus_vdev_driver.h>
>  #include <rte_cycles.h>
>  #include <rte_eal.h>
> @@ -53,7 +55,7 @@
>  	return 0;
>  }
>  
> -static void *
> +static uint32_t
>  cpucopy_thread(void *param)
>  {
>  #define SLEEP_THRESHOLD		10000
> @@ -81,7 +83,7 @@
>  		(void)rte_ring_enqueue(hw->desc_completed, (void *)desc);
>  	}
>  
> -	return NULL;
> +	return 0;
>  }
>  
>  static void
> @@ -126,7 +128,7 @@
>  	rte_mb();
>  
>  	snprintf(name, sizeof(name), "dma_skel_%d", dev->data->dev_id);
> -	ret = rte_ctrl_thread_create(&hw->thread, name, NULL,
> +	ret = rte_thread_create_control(&hw->thread, name, NULL,
>  				     cpucopy_thread, dev);
>  	if (ret) {
>  		SKELDMA_LOG(ERR, "Start cpucopy thread fail!");
> @@ -135,8 +137,7 @@
>  
>  	if (hw->lcore_id != -1) {
>  		cpuset = rte_lcore_cpuset(hw->lcore_id);
> -		ret = pthread_setaffinity_np(hw->thread, sizeof(cpuset),
> -					     &cpuset);
> +		ret = rte_thread_get_affinity_by_id(hw->thread, &cpuset);
>  		if (ret)
>  			SKELDMA_LOG(WARNING,
>  				"Set thread affinity lcore = %d fail!",
> @@ -154,8 +155,8 @@
>  	hw->exit_flag = true;
>  	rte_delay_ms(1);
>  
> -	(void)pthread_cancel(hw->thread);
> -	pthread_join(hw->thread, NULL);
> +	(void)pthread_cancel((pthread_t)hw->thread.opaque_id);
> +	rte_thread_join(hw->thread, NULL);
>  

Is there no rte_* equivalent to pthread_cancel? Will that cause issues
later?

>  	return 0;
>  }
> diff --git a/drivers/dma/skeleton/skeleton_dmadev.h b/drivers/dma/skeleton/skeleton_dmadev.h
> index 6f89400..8670a68 100644
> --- a/drivers/dma/skeleton/skeleton_dmadev.h
> +++ b/drivers/dma/skeleton/skeleton_dmadev.h
> @@ -5,9 +5,9 @@
>  #ifndef SKELETON_DMADEV_H
>  #define SKELETON_DMADEV_H
>  
> -#include <pthread.h>
>  
>  #include <rte_ring.h>
> +#include <rte_thread.h>
>  
>  #define SKELDMA_ARG_LCORE	"lcore"
>  
> @@ -21,7 +21,7 @@ struct skeldma_desc {
>  struct skeldma_hw {
>  	int lcore_id; /* cpucopy task affinity core */
>  	int socket_id;
> -	pthread_t thread; /* cpucopy task thread */
> +	rte_thread_t thread; /* cpucopy task thread */
>  	volatile int exit_flag; /* cpucopy task exit flag */
>  
>  	struct skeldma_desc *desc_mem;
> -- 
> 1.8.3.1
> 

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[Bruce Richardson]

On Sun, Apr 02, 2023 at 10:34:12PM -0700, Tyler Retzlaff wrote:
> early review if possible please, would like to have this in from the
> start of 23.07 to work against.
> 
> thanks!
> 

Don't see any problems with this set.

Series-acked-by: Bruce Richardson <bruce.richardson@intel.com>

> On Fri, Mar 17, 2023 at 03:34:14PM -0700, Tyler Retzlaff wrote:
> > Adopt rte thread APIs in code built for Windows to decouple it from the
> > pthread shim.
> > 
> > Remove most of the pthread_xxx lifetime shim functions, only
> > pthread_create remains while we wait for rte_ctrl_thread_create removal.
> > 
> > Tyler Retzlaff (6):
> >   dma/skeleton: use rte thread API
> >   net/ixgbe: use rte thread API
> >   net/ice: use rte thread API
> >   net/iavf: use rte thread API
> >   eal: use rte thread API
> >   windows: remove most pthread lifetime shim functions
> > 
> >  drivers/dma/skeleton/skeleton_dmadev.c | 15 +++---
> >  drivers/dma/skeleton/skeleton_dmadev.h |  4 +-
> >  drivers/net/iavf/iavf_vchnl.c          | 12 ++---
> >  drivers/net/ice/ice_dcf_parent.c       | 11 ++--
> >  drivers/net/ixgbe/ixgbe_ethdev.c       | 10 ++--
> >  drivers/net/ixgbe/ixgbe_ethdev.h       |  2 +-
> >  lib/eal/common/eal_common_thread.c     |  4 +-
> >  lib/eal/windows/eal.c                  |  2 +-
> >  lib/eal/windows/eal_interrupts.c       | 12 ++---
> >  lib/eal/windows/include/pthread.h      | 99 ----------------------------------
> >  10 files changed, 36 insertions(+), 135 deletions(-)
> > 
> > -- 
> > 1.8.3.1

Re: [PATCH 1/6] dma/skeleton: use rte thread API

[Tyler Retzlaff]

On Tue, Apr 18, 2023 at 10:19:15AM +0100, Bruce Richardson wrote:
> On Fri, Mar 17, 2023 at 03:34:15PM -0700, Tyler Retzlaff wrote:
> > Update driver to use rte thread API where available instead of pthread
> > as a prerequisite to removing pthread stubs on Windows.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  drivers/dma/skeleton/skeleton_dmadev.c | 15 ++++++++-------
> >  drivers/dma/skeleton/skeleton_dmadev.h |  4 ++--
> >  2 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/dma/skeleton/skeleton_dmadev.c b/drivers/dma/skeleton/skeleton_dmadev.c
> > index daf35ec..2ec10db 100644
> > --- a/drivers/dma/skeleton/skeleton_dmadev.c
> > +++ b/drivers/dma/skeleton/skeleton_dmadev.c
> > @@ -5,6 +5,8 @@
> >  #include <inttypes.h>
> >  #include <stdlib.h>
> >  
> > +#include <pthread.h>
> > +
> 
> Curious as to why this is needed here. Does rte_thread.h not include all
> needed threading dependencies?
> [Self-answer] I assume this is for pthread_cancel below, right?

yes, pthread_cancel has no equivalent in the EAL. windows can't easily
provide anything that would behave the same, but pthread_cancel is kind
of awful anyway.

it's something that i'm thinking about but haven't got a good solution
for. so for now any code that uses it remains.

> 
> 
> >  #include <bus_vdev_driver.h>
> >  #include <rte_cycles.h>
> >  #include <rte_eal.h>
> > @@ -53,7 +55,7 @@
> >  	return 0;
> >  }
> >  
> > -static void *
> > +static uint32_t
> >  cpucopy_thread(void *param)
> >  {
> >  #define SLEEP_THRESHOLD		10000
> > @@ -81,7 +83,7 @@
> >  		(void)rte_ring_enqueue(hw->desc_completed, (void *)desc);
> >  	}
> >  
> > -	return NULL;
> > +	return 0;
> >  }
> >  
> >  static void
> > @@ -126,7 +128,7 @@
> >  	rte_mb();
> >  
> >  	snprintf(name, sizeof(name), "dma_skel_%d", dev->data->dev_id);
> > -	ret = rte_ctrl_thread_create(&hw->thread, name, NULL,
> > +	ret = rte_thread_create_control(&hw->thread, name, NULL,
> >  				     cpucopy_thread, dev);
> >  	if (ret) {
> >  		SKELDMA_LOG(ERR, "Start cpucopy thread fail!");
> > @@ -135,8 +137,7 @@
> >  
> >  	if (hw->lcore_id != -1) {
> >  		cpuset = rte_lcore_cpuset(hw->lcore_id);
> > -		ret = pthread_setaffinity_np(hw->thread, sizeof(cpuset),
> > -					     &cpuset);
> > +		ret = rte_thread_get_affinity_by_id(hw->thread, &cpuset);
> >  		if (ret)
> >  			SKELDMA_LOG(WARNING,
> >  				"Set thread affinity lcore = %d fail!",
> > @@ -154,8 +155,8 @@
> >  	hw->exit_flag = true;
> >  	rte_delay_ms(1);
> >  
> > -	(void)pthread_cancel(hw->thread);
> > -	pthread_join(hw->thread, NULL);
> > +	(void)pthread_cancel((pthread_t)hw->thread.opaque_id);
> > +	rte_thread_join(hw->thread, NULL);
> >  
> 
> Is there no rte_* equivalent to pthread_cancel? Will that cause issues
> later?

there isn't because windows can't really do what it is doing sensibly.
in part it's because of how it is influences how other posix calls
behave.

> 
> >  	return 0;
> >  }
> > diff --git a/drivers/dma/skeleton/skeleton_dmadev.h b/drivers/dma/skeleton/skeleton_dmadev.h
> > index 6f89400..8670a68 100644
> > --- a/drivers/dma/skeleton/skeleton_dmadev.h
> > +++ b/drivers/dma/skeleton/skeleton_dmadev.h
> > @@ -5,9 +5,9 @@
> >  #ifndef SKELETON_DMADEV_H
> >  #define SKELETON_DMADEV_H
> >  
> > -#include <pthread.h>
> >  
> >  #include <rte_ring.h>
> > +#include <rte_thread.h>
> >  
> >  #define SKELDMA_ARG_LCORE	"lcore"
> >  
> > @@ -21,7 +21,7 @@ struct skeldma_desc {
> >  struct skeldma_hw {
> >  	int lcore_id; /* cpucopy task affinity core */
> >  	int socket_id;
> > -	pthread_t thread; /* cpucopy task thread */
> > +	rte_thread_t thread; /* cpucopy task thread */
> >  	volatile int exit_flag; /* cpucopy task exit flag */
> >  
> >  	struct skeldma_desc *desc_mem;
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[David Marchand]

On Tue, Apr 18, 2023 at 11:21 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Sun, Apr 02, 2023 at 10:34:12PM -0700, Tyler Retzlaff wrote:
> > early review if possible please, would like to have this in from the
> > start of 23.07 to work against.
> >
> > thanks!
> >
>
> Don't see any problems with this set.

Drivers maintainers were not copied (Tyler, git send-email has options
--to-cmd or --cc-cmd to which you can pass
./devtools/get-maintainers.sh).
I pinged Qi during the maintainers call today.

The changes are straightforward and lgtm.
For the series,
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

RE: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[Zhang, Qi Z]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, June 1, 2023 4:50 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Tyler Retzlaff
> <roretzla@linux.microsoft.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: Re: [PATCH 0/6] windows: remove most pthread lifetime shim
> functions
> 
> On Tue, Apr 18, 2023 at 11:21 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Sun, Apr 02, 2023 at 10:34:12PM -0700, Tyler Retzlaff wrote:
> > > early review if possible please, would like to have this in from the
> > > start of 23.07 to work against.
> > >
> > > thanks!
> > >
> >
> > Don't see any problems with this set.
> 
> Drivers maintainers were not copied (Tyler, git send-email has options --to-
> cmd or --cc-cmd to which you can pass ./devtools/get-maintainers.sh).
> I pinged Qi during the maintainers call today.

Hi Tyler & David:

The patchset looks good to me.

I have just one question regarding the patch set targets, which include PMD iavf, ice, and ixgbe. However, I noticed that some other Intel PMDs like ipn3ke still use rte_ctrl_thread_create and have not been replaced.

I assume that this replacement is specifically intended for PMDs that support Windows, as PMDs with the "Windows" feature should be covered. However, I didn't see the "Windows" feature enabled for iavf PMD, even though it is included in the patch set.

Could you help me understand the criteria used for determining which PMDs should be included in this replacement?

Thanks
Qi 

> 
> The changes are straightforward and lgtm.
> For the series,
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 
> --
> David Marchand

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[Tyler Retzlaff]

On Thu, Jun 01, 2023 at 12:23:39PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, June 1, 2023 4:50 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; Tyler Retzlaff
> > <roretzla@linux.microsoft.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: Re: [PATCH 0/6] windows: remove most pthread lifetime shim
> > functions
> > 
> > On Tue, Apr 18, 2023 at 11:21 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Sun, Apr 02, 2023 at 10:34:12PM -0700, Tyler Retzlaff wrote:
> > > > early review if possible please, would like to have this in from the
> > > > start of 23.07 to work against.
> > > >
> > > > thanks!
> > > >
> > >
> > > Don't see any problems with this set.
> > 
> > Drivers maintainers were not copied (Tyler, git send-email has options --to-
> > cmd or --cc-cmd to which you can pass ./devtools/get-maintainers.sh).
> > I pinged Qi during the maintainers call today.
> 
> Hi Tyler & David:
> 
> The patchset looks good to me.
> 
> I have just one question regarding the patch set targets, which include PMD iavf, ice, and ixgbe. However, I noticed that some other Intel PMDs like ipn3ke still use rte_ctrl_thread_create and have not been replaced.

The series really isn't about rte_ctrl_thread_create, it just happens
that for code built on Windows that code needs to stop using
rte_ctrl_thread_create because it references the pthread shim that is
being removed.

At some point in the future (it's lower priority right now) I will
submit a series that converts all rte_ctrl_thread_create ->
rte_control_thread_create since rte_ctrl_thread_create is being
deprecated.

> 
> I assume that this replacement is specifically intended for PMDs that support Windows, as PMDs with the "Windows" feature should be covered. However, I didn't see the "Windows" feature enabled for iavf PMD, even though it is included in the patch set.
> 
> Could you help me understand the criteria used for determining which PMDs should be included in this replacement?

Yes, you are correct the patch is intended to address PMDs / code built
on Windows specifically. I just re-verified that iavf is being built
for Windows.

If I remove the iavf patch from the series I get the following warning,
so that is why I adapted the iavf PMD code.

  [249/749] Compiling C object
  drivers/libtmp_rte_net_iavf.a.p/net_iavf_iavf_vchnl.c.obj
  ../drivers/net/iavf/iavf_vchnl.c:162:2: warning: implicit declaration of
  function 'pthread_join' is invalid in C99
  [-Wimplicit-function-declaration]
	  pthread_join(handler->tid, NULL);

> 
> Thanks
> Qi 
> 
> > 
> > The changes are straightforward and lgtm.
> > For the series,
> > Reviewed-by: David Marchand <david.marchand@redhat.com>

I think with the above explained the series should be okay as is, no
changes required if Qi is okay with the above explanation.

> > 
> > 
> > --
> > David Marchand
> 

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[David Marchand]

On Fri, Jun 2, 2023 at 6:22 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
> On Thu, Jun 01, 2023 at 12:23:39PM +0000, Zhang, Qi Z wrote:
> > The patchset looks good to me.
> >
> > I have just one question regarding the patch set targets, which include PMD iavf, ice, and ixgbe. However, I noticed that some other Intel PMDs like ipn3ke still use rte_ctrl_thread_create and have not been replaced.
>
> The series really isn't about rte_ctrl_thread_create, it just happens
> that for code built on Windows that code needs to stop using
> rte_ctrl_thread_create because it references the pthread shim that is
> being removed.
>
> At some point in the future (it's lower priority right now) I will
> submit a series that converts all rte_ctrl_thread_create ->
> rte_control_thread_create since rte_ctrl_thread_create is being
> deprecated.
>
> >
> > I assume that this replacement is specifically intended for PMDs that support Windows, as PMDs with the "Windows" feature should be covered. However, I didn't see the "Windows" feature enabled for iavf PMD, even though it is included in the patch set.
> >
> > Could you help me understand the criteria used for determining which PMDs should be included in this replacement?
>
> Yes, you are correct the patch is intended to address PMDs / code built
> on Windows specifically. I just re-verified that iavf is being built
> for Windows.
>
> If I remove the iavf patch from the series I get the following warning,
> so that is why I adapted the iavf PMD code.
>
>   [249/749] Compiling C object
>   drivers/libtmp_rte_net_iavf.a.p/net_iavf_iavf_vchnl.c.obj
>   ../drivers/net/iavf/iavf_vchnl.c:162:2: warning: implicit declaration of
>   function 'pthread_join' is invalid in C99
>   [-Wimplicit-function-declaration]
>           pthread_join(handler->tid, NULL);
>
> >
> > Thanks
> > Qi
> >
> > >
> > > The changes are straightforward and lgtm.
> > > For the series,
> > > Reviewed-by: David Marchand <david.marchand@redhat.com>
>
> I think with the above explained the series should be okay as is, no
> changes required if Qi is okay with the above explanation.

Which seems to be the case.
Thank you.


-- 
David Marchand

Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

[David Marchand]

On Fri, Mar 17, 2023 at 11:35 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Adopt rte thread APIs in code built for Windows to decouple it from the
> pthread shim.
>
> Remove most of the pthread_xxx lifetime shim functions, only
> pthread_create remains while we wait for rte_ctrl_thread_create removal.
>
> Tyler Retzlaff (6):
>   dma/skeleton: use rte thread API
>   net/ixgbe: use rte thread API
>   net/ice: use rte thread API
>   net/iavf: use rte thread API
>   eal: use rte thread API
>   windows: remove most pthread lifetime shim functions

Series applied, thanks Tyler.


-- 
David Marchand