diff mbox series

[v7,1/5] eal: add sleep API

Message ID 20210403234129.20296-2-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series eal/windows: do not expose POSIX symbols | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk April 3, 2021, 11:41 p.m. UTC
POSIX sleep(3) is missing from Windows.
Add generic rte_thread_sleep() to suspend current OS thread.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Acked-by: Khoa To <khot@microsoft.com>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
---
 lib/librte_eal/common/eal_common_timer.c |  5 +++--
 lib/librte_eal/include/rte_thread.h      | 11 +++++++++++
 lib/librte_eal/rte_eal_exports.def       |  2 ++
 lib/librte_eal/unix/rte_thread.c         | 10 +++++++++-
 lib/librte_eal/version.map               |  1 +
 lib/librte_eal/windows/eal_thread.c      |  9 ++++++++-
 6 files changed, 34 insertions(+), 4 deletions(-)

Comments

Morten Brørup April 6, 2021, 2:34 p.m. UTC | #1
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dmitry Kozlyuk
> Sent: Sunday, April 4, 2021 1:41 AM
> 
> POSIX sleep(3) is missing from Windows.
> Add generic rte_thread_sleep() to suspend current OS thread.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> Acked-by: Khoa To <khot@microsoft.com>
> Acked-by: Ray Kinsella <mdr@ashroe.eu>

How is this better than the rte_delay_us_sleep() function in rte_cycles.h?


Med venlig hilsen / kind regards
- Morten Brørup
Dmitry Kozlyuk April 6, 2021, 11:29 p.m. UTC | #2
2021-04-06 16:34 (UTC+0200), Morten Brørup:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dmitry Kozlyuk
> > Sent: Sunday, April 4, 2021 1:41 AM
> > 
> > POSIX sleep(3) is missing from Windows.
> > Add generic rte_thread_sleep() to suspend current OS thread.
> > 
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > Acked-by: Khoa To <khot@microsoft.com>
> > Acked-by: Ray Kinsella <mdr@ashroe.eu>  
> 
> How is this better than the rte_delay_us_sleep() function in rte_cycles.

Honestly, I just assumed the sleep() shim existed for a reason.
Now that you pointed to it, I think indeed no new API is needed.

I wonder why rte_delay_us_sleep() is not used throughout DPDK.
Should their usages be converted as soon as code is enabled for Windows?
Morten Brørup April 7, 2021, 7:31 a.m. UTC | #3
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dmitry Kozlyuk
> Sent: Wednesday, April 7, 2021 1:30 AM
> 
> 2021-04-06 16:34 (UTC+0200), Morten Brørup:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dmitry Kozlyuk
> > > Sent: Sunday, April 4, 2021 1:41 AM
> > >
> > > POSIX sleep(3) is missing from Windows.
> > > Add generic rte_thread_sleep() to suspend current OS thread.
> > >
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > Acked-by: Khoa To <khot@microsoft.com>
> > > Acked-by: Ray Kinsella <mdr@ashroe.eu>
> >
> > How is this better than the rte_delay_us_sleep() function in
> rte_cycles.
> 
> Honestly, I just assumed the sleep() shim existed for a reason.
> Now that you pointed to it, I think indeed no new API is needed.
> 
> I wonder why rte_delay_us_sleep() is not used throughout DPDK.

I think it's just tradition from Linux/BSD developers not being used to cross-platform development or having forgotten how to develop software for deeply embedded systems without a standard O/S below. We use POSIX functions like sleep() without thinking about it, and simply forget looking for an EAL variant of the function first.

> Should their usages be converted as soon as code is enabled for
> Windows?

Yes. The Environment Abstraction Layer is there to avoid using O/S specific function calls. So it should be safe to replace all these sleep() calls with the EAL variant at any time.
Tyler Retzlaff April 29, 2021, 5:09 p.m. UTC | #4
On Wed, Apr 07, 2021 at 09:31:43AM +0200, Morten Brørup wrote:
> 
> I think it's just tradition from Linux/BSD developers not being used to cross-platform development or having forgotten how to develop software for deeply embedded systems without a standard O/S below. We use POSIX functions like sleep() without thinking about it, and simply forget looking for an EAL variant of the function first.
> 

it's easy to make this mistake if by including dpdk application headers
we indirectly introduce platform specific headers into the applications
namespace.

if this wasn't happening the application would be forced to confront the
issue by directly including unistd.h itself directly making it more
obvious that they were writing inherently non-portable code.

i think this is being fixed but it is a work in progress but it
highlights the reason why there needs to be scrutiny around what goes
into the eal and other library headers.
diff mbox series

Patch

diff --git a/lib/librte_eal/common/eal_common_timer.c b/lib/librte_eal/common/eal_common_timer.c
index 71e0bd035a..0e89a4f7df 100644
--- a/lib/librte_eal/common/eal_common_timer.c
+++ b/lib/librte_eal/common/eal_common_timer.c
@@ -16,6 +16,7 @@ 
 #include <rte_cycles.h>
 #include <rte_pause.h>
 #include <rte_eal.h>
+#include <rte_thread.h>
 
 #include "eal_private.h"
 #include "eal_memcfg.h"
@@ -47,9 +48,9 @@  estimate_tsc_freq(void)
 #define CYC_PER_10MHZ 1E7
 	RTE_LOG(WARNING, EAL, "WARNING: TSC frequency estimated roughly"
 		" - clock timings may be less accurate.\n");
-	/* assume that the sleep(1) will sleep for 1 second */
+	/* assume that the rte_thread_sleep(1) will sleep for 1 second */
 	uint64_t start = rte_rdtsc();
-	sleep(1);
+	rte_thread_sleep(1);
 	/* Round up to 10Mhz. 1E7 ~ 10Mhz */
 	return RTE_ALIGN_MUL_NEAR(rte_rdtsc() - start, CYC_PER_10MHZ);
 }
diff --git a/lib/librte_eal/include/rte_thread.h b/lib/librte_eal/include/rte_thread.h
index 8be8ed8f36..450d72a2fc 100644
--- a/lib/librte_eal/include/rte_thread.h
+++ b/lib/librte_eal/include/rte_thread.h
@@ -119,6 +119,17 @@  int rte_thread_value_set(rte_thread_key key, const void *value);
 __rte_experimental
 void *rte_thread_value_get(rte_thread_key key);
 
+/**
+ * Suspend current OS thread for the specified time, yielding CPU to scheduler.
+ *
+ * @param sec
+ *  Number of seconds to sleep. The system may return control later,
+ *  but not earlier. Zero value always yields the CPU, but control may be
+ *  returned immediately.
+ */
+__rte_experimental
+void rte_thread_sleep(unsigned int sec);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
index c320077547..300c86eb3f 100644
--- a/lib/librte_eal/rte_eal_exports.def
+++ b/lib/librte_eal/rte_eal_exports.def
@@ -334,3 +334,5 @@  EXPORTS
 	rte_mem_map
 	rte_mem_page_size
 	rte_mem_unmap
+
+	rte_thread_sleep
diff --git a/lib/librte_eal/unix/rte_thread.c b/lib/librte_eal/unix/rte_thread.c
index c72d619ec1..a7130b5870 100644
--- a/lib/librte_eal/unix/rte_thread.c
+++ b/lib/librte_eal/unix/rte_thread.c
@@ -3,10 +3,12 @@ 
  */
 
 #include <errno.h>
-#include <pthread.h>
 #include <stdlib.h>
 #include <string.h>
 
+#include <pthread.h>
+#include <unistd.h>
+
 #include <rte_common.h>
 #include <rte_errno.h>
 #include <rte_log.h>
@@ -90,3 +92,9 @@  rte_thread_value_get(rte_thread_key key)
 	}
 	return pthread_getspecific(key->thread_index);
 }
+
+void
+rte_thread_sleep(unsigned int sec)
+{
+	sleep(sec);
+}
diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map
index e23745ae6e..cd35b67a9a 100644
--- a/lib/librte_eal/version.map
+++ b/lib/librte_eal/version.map
@@ -413,6 +413,7 @@  EXPERIMENTAL {
 	# added in 21.05
 	rte_thread_key_create;
 	rte_thread_key_delete;
+	rte_thread_sleep;
 	rte_thread_value_get;
 	rte_thread_value_set;
 	rte_version_minor;
diff --git a/lib/librte_eal/windows/eal_thread.c b/lib/librte_eal/windows/eal_thread.c
index 9c3f6d69fd..c84e67009c 100644
--- a/lib/librte_eal/windows/eal_thread.c
+++ b/lib/librte_eal/windows/eal_thread.c
@@ -11,9 +11,10 @@ 
 #include <rte_per_lcore.h>
 #include <rte_common.h>
 #include <rte_memory.h>
-#include <eal_thread.h>
+#include <rte_thread.h>
 
 #include "eal_private.h"
+#include "eal_thread.h"
 #include "eal_windows.h"
 
 /*
@@ -154,3 +155,9 @@  rte_thread_setname(__rte_unused pthread_t id, __rte_unused const char *name)
 	/* This is a stub, not the expected result */
 	return 0;
 }
+
+void
+rte_thread_sleep(unsigned int sec)
+{
+	return Sleep(MS_PER_S * sec);
+}