[dpdk-dev] [PATCH] eal/windows: refine public interface

[dpdk-dev] [PATCH] eal/windows: refine public interface

[Dmitry Kozlyuk]

The goal of rte_os.h is to mitigate OS differences for EAL users.
In Windows EAL, rte_os.h did excessive things:

1. It included platform SDK headers (windows.h, etc). Those files are
   huge, require specific inclusion order, and are generally unused by
   the code including rte_os.h. Declarations from platform SDK may
   break otherwise platform-independent code, e.g. min, max, ERROR.

2. It included pthread.h, which is clearly not always required.

3. It defined functions private to Windows EAL.

Reorganize Windows EAL includes in the following way:

1. Create rte_windows.h to properly import Windows-specific facilities.
   Primary users are bus drivers, tests, and external applications.

2. Remove platform SDK includes from rte_os.h to make it portable.
   Copy necessary definitions to avoid including those headers.

3. Remove pthread.h include from rte_os.h.

4. Move declarations private to Windows EAL into eal_windows.h.

Fixes: 428eb983 ("eal: add OS specific header file")
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/windows/eal/eal.c              |  2 ++
 lib/librte_eal/windows/eal/eal_lcore.c        |  3 ++
 lib/librte_eal/windows/eal/eal_thread.c       |  1 +
 lib/librte_eal/windows/eal/eal_windows.h      | 29 +++++++++++++++++++
 lib/librte_eal/windows/eal/include/pthread.h  |  2 ++
 lib/librte_eal/windows/eal/include/rte_os.h   | 26 ++---------------
 .../windows/eal/include/rte_windows.h         | 24 +++++++++++++++
 lib/librte_eal/windows/eal/meson.build        |  1 +
 8 files changed, 65 insertions(+), 23 deletions(-)
 create mode 100644 lib/librte_eal/windows/eal/eal_windows.h
 create mode 100644 lib/librte_eal/windows/eal/include/rte_windows.h

diff --git a/lib/librte_eal/windows/eal/eal.c b/lib/librte_eal/windows/eal/eal.c
index ce460481f..25b0c341c 100644
--- a/lib/librte_eal/windows/eal/eal.c
+++ b/lib/librte_eal/windows/eal/eal.c
@@ -11,6 +11,8 @@
 #include <eal_thread.h>
 #include <eal_private.h>
 
+#include "eal_windows.h"
+
 /* Address of global and public configuration */
 static struct rte_config rte_config;
 
diff --git a/lib/librte_eal/windows/eal/eal_lcore.c b/lib/librte_eal/windows/eal/eal_lcore.c
index d39f348a3..7a81127d3 100644
--- a/lib/librte_eal/windows/eal/eal_lcore.c
+++ b/lib/librte_eal/windows/eal/eal_lcore.c
@@ -2,10 +2,13 @@
  * Copyright(c) 2019 Intel Corporation
  */
 
+#include <pthread.h>
 #include <stdint.h>
 
 #include <rte_common.h>
 
+#include "eal_windows.h"
+
 /* global data structure that contains the CPU map */
 static struct _wcpu_map {
 	unsigned int total_procs;
diff --git a/lib/librte_eal/windows/eal/eal_thread.c b/lib/librte_eal/windows/eal/eal_thread.c
index 0591d4c7f..0d9127804 100644
--- a/lib/librte_eal/windows/eal/eal_thread.c
+++ b/lib/librte_eal/windows/eal/eal_thread.c
@@ -13,6 +13,7 @@
 #include <eal_thread.h>
 
 #include "eal_private.h"
+#include "eal_windows.h"
 
 RTE_DEFINE_PER_LCORE(unsigned int, _lcore_id) = LCORE_ID_ANY;
 
diff --git a/lib/librte_eal/windows/eal/eal_windows.h b/lib/librte_eal/windows/eal/eal_windows.h
new file mode 100644
index 000000000..fadd676b2
--- /dev/null
+++ b/lib/librte_eal/windows/eal/eal_windows.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Dmitry Kozlyuk
+ */
+
+#ifndef _EAL_WINDOWS_H_
+#define _EAL_WINDOWS_H_
+
+/**
+ * @file Facilities private to Windows EAL
+ */
+
+#include <rte_windows.h>
+
+/**
+ * Create a map of processors and cores on the system.
+ */
+void eal_create_cpu_map(void);
+
+/**
+ * Create a thread.
+ *
+ * @param thread
+ *   The location to store the thread id if successful.
+ * @return
+ *   0 for success, -1 if the thread is not created.
+ */
+int eal_thread_create(pthread_t *thread);
+
+#endif /* _EAL_WINDOWS_H_ */
diff --git a/lib/librte_eal/windows/eal/include/pthread.h b/lib/librte_eal/windows/eal/include/pthread.h
index 503329266..c181a59f7 100644
--- a/lib/librte_eal/windows/eal/include/pthread.h
+++ b/lib/librte_eal/windows/eal/include/pthread.h
@@ -5,6 +5,8 @@
 #ifndef _PTHREAD_H_
 #define _PTHREAD_H_
 
+#include <stdint.h>
+
 /**
  * This file is required to support the common code in eal_common_proc.c,
  * eal_common_thread.c and common\include\rte_per_lcore.h as Microsoft libc
diff --git a/lib/librte_eal/windows/eal/include/rte_os.h b/lib/librte_eal/windows/eal/include/rte_os.h
index fdeae0c8f..d9485d44d 100644
--- a/lib/librte_eal/windows/eal/include/rte_os.h
+++ b/lib/librte_eal/windows/eal/include/rte_os.h
@@ -8,43 +8,23 @@
 /**
  * This is header should contain any function/macro definition
  * which are not supported natively or named differently in the
- * Windows OS. Functions will be added in future releases.
+ * Windows OS. It must not include Windows-specific headers.
  */
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-#include <Windows.h>
-#include <BaseTsd.h>
-#include <pthread.h>
-
 #define strerror_r(a, b, c) strerror_s(b, c, a)
 
 /* strdup is deprecated in Microsoft libc and _strdup is preferred */
 #define strdup(str) _strdup(str)
 
-typedef SSIZE_T ssize_t;
+/* as in <windows.h> */
+typedef long long ssize_t;
 
 #define strtok_r(str, delim, saveptr) strtok_s(str, delim, saveptr)
 
-/**
- * Create a thread.
- * This function is private to EAL.
- *
- * @param thread
- *   The location to store the thread id if successful.
- * @return
- *   0 for success, -1 if the thread is not created.
- */
-int eal_thread_create(pthread_t *thread);
-
-/**
- * Create a map of processors and cores on the system.
- * This function is private to EAL.
- */
-void eal_create_cpu_map(void);
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_eal/windows/eal/include/rte_windows.h b/lib/librte_eal/windows/eal/include/rte_windows.h
new file mode 100644
index 000000000..1cc9ddc35
--- /dev/null
+++ b/lib/librte_eal/windows/eal/include/rte_windows.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Dmitry Kozlyuk
+ */
+
+#ifndef _RTE_WINDOWS_H_
+#define _RTE_WINDOWS_H_
+
+/**
+ * @file Windows-specific facilities
+ *
+ * This file should be included by DPDK libraries and applications
+ * that need access to Windows API. It includes platform SDK headers
+ * in compatible order and with proper options.
+ *
+ * Future versions may include macros for Windows-specific error handling.
+ */
+
+#define WIN32_LEAN_AND_MEAN /* Disable excessive libraries. */
+#define INITGUID            /* Have GUIDs defined. */
+
+#include <windows.h>
+#include <basetsd.h>
+
+#endif /* _RTE_WINDOWS_H_ */
diff --git a/lib/librte_eal/windows/eal/meson.build b/lib/librte_eal/windows/eal/meson.build
index af4f70f00..dd6721c57 100644
--- a/lib/librte_eal/windows/eal/meson.build
+++ b/lib/librte_eal/windows/eal/meson.build
@@ -6,6 +6,7 @@ eal_inc += include_directories('include')
 env_objs = []
 env_headers = files(
 	'include/rte_os.h',
+	'include/rte_windows.h',
 )
 common_sources = files(
 	'../../common/eal_common_errno.c',
-- 
2.25.0

Re: [dpdk-dev] [PATCH] eal/windows: refine public interface

[Thomas Monjalon]

31/01/2020 06:24, Dmitry Kozlyuk:
> The goal of rte_os.h is to mitigate OS differences for EAL users.
> In Windows EAL, rte_os.h did excessive things:

That's an interesting point of view.
The idea of rte_os.h was to hide OS-specifics, so the DPDK applications
have no Windows-specific header to include for using DPDK.
The secondary goal is to avoid Windows-specific includes in DPDK libs
and drivers.

I agree bloating rte_os.h is a concern.
If you can achieve the initial goal (no specific include in apps, libs
and drivers) while reducing rte_os.h, I think it is good.


> 1. It included platform SDK headers (windows.h, etc). Those files are
>    huge, require specific inclusion order, and are generally unused by
>    the code including rte_os.h. Declarations from platform SDK may
>    break otherwise platform-independent code, e.g. min, max, ERROR.
> 
> 2. It included pthread.h, which is clearly not always required.
> 
> 3. It defined functions private to Windows EAL.
> 
> Reorganize Windows EAL includes in the following way:
> 
> 1. Create rte_windows.h to properly import Windows-specific facilities.
>    Primary users are bus drivers, tests, and external applications.
> 
> 2. Remove platform SDK includes from rte_os.h to make it portable.

This file is compiled only on Windows.
What do you mean with "portable"?

>    Copy necessary definitions to avoid including those headers.
> 
> 3. Remove pthread.h include from rte_os.h.
> 
> 4. Move declarations private to Windows EAL into eal_windows.h.
> 
> Fixes: 428eb983 ("eal: add OS specific header file")

nit: blank line missing here

> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH] eal/windows: refine public interface

[Dmitry Kozlyuk]

> I agree bloating rte_os.h is a concern.
> If you can achieve the initial goal (no specific include in apps, libs
> and drivers) while reducing rte_os.h, I think it is good.

<rte_windows.h> would be such a specific include, like <rte_kni_common.h> in
Linux EAL. This file would include Windows platform SDK files in the right
order, define feature macros and maybe error-handling macros, so that EAL
users could just access Windows SDK in one line without being bothered by
details and having handy macros as a bonus.


> This file is compiled only on Windows.
> What do you mean with "portable"?

By "portable" I mean not breaking OS-independent code with Windows defines.
Item 1 gives "min", "max" and "ERROR" macros as examples. Also, some Windows
includes must come after the others, e.g. <winsock2.h> (Berkeley sockets) must
come strictly after <windows.h>, and it's not an isolated case.


> > Fixes: 428eb983 ("eal: add OS specific header file")  
> 
> nit: blank line missing here

Thanks, I've little experience sending patches.

-- 
Dmitry Kozlyuk