[2/2] eal/windows: fix build by supporting trace
Checks
Commit Message
Add EAL private functions to support trace storage:
* eal_persistent_data_path()
* eal_dir_create()
Replace clock_gettime(CLOCK_REALTIME) with C11 timespec_get().
Implementation is provided for MinGW-w64 that misses this function.
Provide minimum viable implementations of malloc and timer functions
used by tracing.
Fixes: 185b7dc1d467 ("trace: save bootup timestamp")
Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
config/meson.build | 2 +
.../common/eal_common_trace_utils.c | 29 ++----
lib/librte_eal/common/eal_private.h | 26 ++++++
lib/librte_eal/common/meson.build | 5 +
lib/librte_eal/freebsd/Makefile | 4 +
.../include/generic/rte_byteorder.h | 4 +-
lib/librte_eal/linux/Makefile | 4 +
lib/librte_eal/meson.build | 4 +
lib/librte_eal/unix/eal_unix.c | 45 +++++++++
lib/librte_eal/unix/meson.build | 6 ++
lib/librte_eal/windows/eal.c | 91 +++++++++++++++++++
lib/librte_eal/windows/eal_thread.c | 9 ++
lib/librte_eal/windows/eal_windows.h | 3 +
lib/librte_eal/windows/include/rte_os.h | 33 +++++--
14 files changed, 237 insertions(+), 28 deletions(-)
create mode 100644 lib/librte_eal/unix/eal_unix.c
create mode 100644 lib/librte_eal/unix/meson.build
Comments
On Sun, Apr 26, 2020 at 8:53 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> Add EAL private functions to support trace storage:
>
> * eal_persistent_data_path()
> * eal_dir_create()
>
> Replace clock_gettime(CLOCK_REALTIME) with C11 timespec_get().
> Implementation is provided for MinGW-w64 that misses this function.
>
> Provide minimum viable implementations of malloc and timer functions
> used by tracing.
>
> Fixes: 185b7dc1d467 ("trace: save bootup timestamp")
> Fixes: 321dd5f8fa62 ("trace: add internal init and fini interface")
> Reported-by: Pallavi Kadam <pallavi.kadam@intel.com>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
In general, the patch looks good to me. Could you split the patch as two
1) New eal_permanent_data_path() and eal_dir_create() API definition,
and its implementation with unix and the common code changes to adapt new APIs.
2) Windows implementation of eal_permanent_data_path() and other
missing pieces for
trace implementation.
Some comments below,
>
> void eal_free_no_trace(void *addr);
>
> +/**
> + * Get absolute path to the directory where permanent data can be stored.
> + *
> + * @return
> + * Statically allocated string on success, NULL on failure.
> + */
> +const char *
> +eal_permanent_data_path(void);
Do windows have PATH_MAX kind of macro? I think, it is better API
consumer allocates
the memory of size PATH_MAX and implementation fills it, instead of,
the static scheme.
> +
> +/**
> + * Create a directory accessible to the current user only.
> + *
> + * This function does not create intermediate directories,
> + * thus only the last path component may be nonexistent.
> + *
> + * This function succeeds if path already exists and is a directory.
> + *
> + * Platform-independent code should use forward slash as path separator.
> + *
> + * @param path
> + * Path to be created.
> + * @return
> + * 0 on success, (-1) on failure and rte_errno is set.
> + */
> +int eal_dir_create(const char *path);
>
Looks good to me.
On 2020-04-26 17:02 GMT+0530 Jerin Jacob wrote:
> > +/**
> > + * Get absolute path to the directory where permanent data can be stored.
> > + *
> > + * @return
> > + * Statically allocated string on success, NULL on failure.
> > + */
> > +const char *
> > +eal_permanent_data_path(void);
>
> Do windows have PATH_MAX kind of macro? I think, it is better API
> consumer allocates
> the memory of size PATH_MAX and implementation fills it, instead of,
> the static scheme.
This API falls in line with rte_eal_get_runtime_dir() and other
eal_filesystem.h functions, that use static scheme. Logically, its result
never changes. It is race-free and is only called during initialization. What
you propose can be done, but are there any benefits?
While we're at it, don't these declarations belong to eal_filesystem.h? I
left them in eal_private.h, because eal_filesystem.h is mostly Unix-specific.
On Sun, Apr 26, 2020 at 5:32 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> On 2020-04-26 17:02 GMT+0530 Jerin Jacob wrote:
> > > +/**
> > > + * Get absolute path to the directory where permanent data can be stored.
> > > + *
> > > + * @return
> > > + * Statically allocated string on success, NULL on failure.
> > > + */
> > > +const char *
> > > +eal_permanent_data_path(void);
> >
> > Do windows have PATH_MAX kind of macro? I think, it is better API
> > consumer allocates
> > the memory of size PATH_MAX and implementation fills it, instead of,
> > the static scheme.
>
> This API falls in line with rte_eal_get_runtime_dir() and other
> eal_filesystem.h functions, that use static scheme. Logically, its result
> never changes. It is race-free and is only called during initialization. What
> you propose can be done, but are there any benefits?
I thought who will free that memory? It looks like libc creating a static memory
for this item. so, your current eal_permanent_data_path() declarations
looks good to
me.
>
> While we're at it, don't these declarations belong to eal_filesystem.h? I
> left them in eal_private.h, because eal_filesystem.h is mostly Unix-specific.
Understood, Leaving that decision to EAL maintainers.
>
> --
> Dmitry Kozlyuk
26/04/2020 14:23, Jerin Jacob:
> On Sun, Apr 26, 2020 at 5:32 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > While we're at it, don't these declarations belong to eal_filesystem.h? I
> > left them in eal_private.h, because eal_filesystem.h is mostly Unix-specific.
Yes it looks to be a good fit for eal_filesystem.h.
Don't hesitate removing Linux specifics (starting with the file comment).
> Understood, Leaving that decision to EAL maintainers.
There is no EAL maintainer for this.
On 2020-04-26 17:53 GMT+0530 Jerin Jacob wrote:
> On Sun, Apr 26, 2020 at 5:32 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > On 2020-04-26 17:02 GMT+0530 Jerin Jacob wrote:
> > > > +/**
> > > > + * Get absolute path to the directory where permanent data can be stored.
> > > > + *
> > > > + * @return
> > > > + * Statically allocated string on success, NULL on failure.
> > > > + */
> > > > +const char *
> > > > +eal_permanent_data_path(void);
> > >
> > > Do windows have PATH_MAX kind of macro? I think, it is better API
> > > consumer allocates
> > > the memory of size PATH_MAX and implementation fills it, instead of,
> > > the static scheme.
> >
> > This API falls in line with rte_eal_get_runtime_dir() and other
> > eal_filesystem.h functions, that use static scheme. Logically, its result
> > never changes. It is race-free and is only called during initialization. What
> > you propose can be done, but are there any benefits?
>
> I thought who will free that memory? It looks like libc creating a static memory
> for this item. so, your current eal_permanent_data_path() declarations
> looks good to
> me.
Actually, your concern is valid (see man quotes below). Windows implementation
has no such issue because of its own static buffer. I'd use static scheme
in EAL interface for convenience, but change Unix implementation to copy
string returned by libc into a static buffer.
man 3 getenv:
The string pointed to by the return value of getenv() may be
statically allocated, and can be modified by a subsequent call to
getenv(), putenv(3), setenv(3), or unsetenv(3).
man 3 getpwuid:
The return value may point to a static area, and may be overwritten
by subsequent calls to getpwent(3), getpwnam(), or getpwuid().
@@ -267,6 +267,8 @@ if is_windows
# Minimum supported API is Windows 7.
add_project_arguments('-D_WIN32_WINNT=0x0601', language: 'c')
+ add_project_link_arguments(['-lshell32', '-lshlwapi'], language: 'c')
+
# Use MinGW-w64 stdio, because DPDK assumes ANSI-compliant formatting.
if cc.get_id() == 'gcc'
add_project_arguments('-D__USE_MINGW_ANSI_STDIO', language: 'c')
@@ -3,12 +3,11 @@
*/
#include <fnmatch.h>
-#include <pwd.h>
-#include <sys/stat.h>
#include <time.h>
#include <rte_common.h>
#include <rte_errno.h>
+#include <rte_os.h>
#include <rte_string_fns.h>
#include "eal_filesystem.h"
@@ -302,7 +301,7 @@ trace_epoch_time_save(void)
uint64_t avg, start, end;
start = rte_get_tsc_cycles();
- if (clock_gettime(CLOCK_REALTIME, &epoch) < 0) {
+ if (timespec_get(&epoch, TIME_UTC) < 0) {
trace_err("failed to get the epoch time");
return -1;
}
@@ -321,22 +320,14 @@ trace_dir_default_path_get(char *dir_path)
{
struct trace *trace = trace_obj_get();
uint32_t size = sizeof(trace->dir);
- struct passwd *pwd;
- char *home_dir;
-
- /* First check for shell environment variable */
- home_dir = getenv("HOME");
- if (home_dir == NULL) {
- /* Fallback to password file entry */
- pwd = getpwuid(getuid());
- if (pwd == NULL)
- return -EINVAL;
-
- home_dir = pwd->pw_dir;
- }
+ const char *perm_dir;
+
+ perm_dir = eal_permanent_data_path();
+ if (perm_dir == NULL)
+ return -EINVAL;
/* Append dpdk-traces to directory */
- if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
+ if (snprintf(dir_path, size, "%s/dpdk-traces/", perm_dir) < 0)
return -ENAMETOOLONG;
return 0;
@@ -371,7 +362,7 @@ trace_mkdir(void)
}
/* Create the path if it t exist, no "mkdir -p" available here */
- rc = mkdir(trace->dir, 0700);
+ rc = eal_dir_create(trace->dir);
if (rc < 0 && errno != EEXIST) {
trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
rte_errno = errno;
@@ -385,7 +376,7 @@ trace_mkdir(void)
if (rc < 0)
return rc;
- rc = mkdir(trace->dir, 0700);
+ rc = eal_dir_create(trace->dir);
if (rc < 0) {
trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
rte_errno = errno;
@@ -448,4 +448,30 @@ eal_malloc_no_trace(const char *type, size_t size, unsigned int align);
void eal_free_no_trace(void *addr);
+/**
+ * Get absolute path to the directory where permanent data can be stored.
+ *
+ * @return
+ * Statically allocated string on success, NULL on failure.
+ */
+const char *
+eal_permanent_data_path(void);
+
+/**
+ * Create a directory accessible to the current user only.
+ *
+ * This function does not create intermediate directories,
+ * thus only the last path component may be nonexistent.
+ *
+ * This function succeeds if path already exists and is a directory.
+ *
+ * Platform-independent code should use forward slash as path separator.
+ *
+ * @param path
+ * Path to be created.
+ * @return
+ * 0 on success, (-1) on failure and rte_errno is set.
+ */
+int eal_dir_create(const char *path);
+
#endif /* _EAL_PRIVATE_H_ */
@@ -14,6 +14,11 @@ if is_windows
'eal_common_log.c',
'eal_common_options.c',
'eal_common_thread.c',
+ 'eal_common_trace.c',
+ 'eal_common_trace_ctf.c',
+ 'eal_common_trace_utils.c',
+ 'eal_common_string_fns.c',
+ 'eal_common_uuid.c',
'rte_option.c',
)
subdir_done()
@@ -7,6 +7,7 @@ LIB = librte_eal.a
ARCH_DIR ?= $(RTE_ARCH)
VPATH += $(RTE_SDK)/lib/librte_eal/$(ARCH_DIR)
+VPATH += $(RTE_SDK)/lib/librte_eal/unix
VPATH += $(RTE_SDK)/lib/librte_eal/common
CFLAGS += -I$(SRCDIR)/include
@@ -74,6 +75,9 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_service.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_random.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_reciprocal.c
+# from unix dir
+SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += eal_unix.c
+
# from arch dir
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_cpuflags.c
SRCS-$(CONFIG_RTE_EXEC_ENV_FREEBSD) += rte_hypervisor.c
@@ -15,9 +15,9 @@
*/
#include <stdint.h>
-#ifdef RTE_EXEC_ENV_FREEBSD
+#if defined(RTE_EXEC_ENV_FREEBSD)
#include <sys/endian.h>
-#else
+#elif defined(RTE_EXEC_ENV_LINUX)
#include <endian.h>
#endif
@@ -7,6 +7,7 @@ LIB = librte_eal.a
ARCH_DIR ?= $(RTE_ARCH)
VPATH += $(RTE_SDK)/lib/librte_eal/$(ARCH_DIR)
+VPATH += $(RTE_SDK)/lib/librte_eal/unix
VPATH += $(RTE_SDK)/lib/librte_eal/common
CFLAGS += -I$(SRCDIR)/include
@@ -81,6 +82,9 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_service.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_random.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_reciprocal.c
+# from unix dir
+SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += eal_unix.c
+
# from arch dir
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_cpuflags.c
SRCS-$(CONFIG_RTE_EXEC_ENV_LINUX) += rte_hypervisor.c
@@ -6,6 +6,10 @@ subdir('include')
subdir('common')
+if not is_windows
+ subdir('unix')
+endif
+
dpdk_conf.set('RTE_EXEC_ENV_' + exec_env.to_upper(), 1)
subdir(exec_env)
new file mode 100644
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2020 Dmitry Kozlyuk
+ */
+
+#include <stdlib.h>
+
+#include <pwd.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <rte_errno.h>
+
+#include <eal_private.h>
+
+const char *
+eal_permanent_data_path(void)
+{
+ static const char *home_dir; /* static so auto-zeroed */
+
+ struct passwd *pwd;
+
+ if (home_dir != NULL)
+ return home_dir;
+
+ /* First check for shell environment variable */
+ home_dir = getenv("HOME");
+ if (home_dir == NULL) {
+ /* Fallback to password file entry */
+ pwd = getpwuid(getuid());
+ if (pwd == NULL)
+ return NULL;
+
+ home_dir = pwd->pw_dir;
+ }
+ return home_dir;
+}
+
+int
+eal_dir_create(const char *path)
+{
+ int ret = mkdir(path, 0700);
+ if (ret)
+ rte_errno = errno;
+ return ret;
+}
new file mode 100644
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright (c) 2020 Dmitry Kozlyuk
+
+sources += files(
+ 'eal_unix.c',
+)
@@ -17,6 +17,7 @@
#include <eal_filesystem.h>
#include <eal_options.h>
#include <eal_private.h>
+#include <eal_trace.h>
#include "eal_windows.h"
@@ -208,6 +209,90 @@ eal_parse_args(int argc, char **argv)
return ret;
}
+void *
+eal_malloc_no_trace(const char *type, size_t size, unsigned int align)
+{
+ /* Simulate failure, so that tracing falls back to malloc(). */
+ RTE_SET_USED(type);
+ RTE_SET_USED(size);
+ RTE_SET_USED(align);
+ return NULL;
+}
+
+void
+eal_free_no_trace(void *addr __rte_unused)
+{
+ /* Nothing to free. */
+}
+
+/* MinGW-w64 does not implement timespec_get(). */
+#ifdef RTE_TOOLCHAIN_GCC
+
+int
+timespec_get(struct timespec *ts, int base)
+{
+ static const uint64_t UNITS_PER_SEC = 10000000; /* 1 unit = 100 ns */
+
+ FILETIME ft;
+ ULARGE_INTEGER ui;
+
+ GetSystemTimePreciseAsFileTime(&ft);
+ ui.LowPart = ft.dwLowDateTime;
+ ui.HighPart = ft.dwHighDateTime;
+ ts->tv_sec = ui.QuadPart / UNITS_PER_SEC;
+ ts->tv_nsec = ui.QuadPart - (ts->tv_sec * UNITS_PER_SEC);
+ return base;
+}
+
+#endif /* RTE_TOOLCHAIN_GCC */
+
+uint64_t
+rte_get_tsc_hz(void)
+{
+ static LARGE_INTEGER freq; /* static so auto-zeroed */
+
+ if (freq.QuadPart != 0)
+ return freq.QuadPart;
+
+ QueryPerformanceFrequency(&freq);
+ return freq.QuadPart;
+}
+
+const char *
+eal_permanent_data_path(void)
+{
+ static char buffer[MAX_PATH]; /* static so auto-zeroed */
+ HRESULT ret;
+
+ if (buffer[0] != '\0')
+ return buffer;
+
+ ret = SHGetFolderPathA(
+ NULL, CSIDL_LOCAL_APPDATA, NULL, SHGFP_TYPE_CURRENT, buffer);
+ if (FAILED(ret)) {
+ RTE_LOG_WIN32_ERR("SHGetFolderPathA(CSIDL_LOCAL_APPDATA)");
+ return NULL;
+ }
+
+ return buffer;
+}
+
+int
+eal_dir_create(const char *path)
+{
+ /* CreateDirectoryA() fails if directory exists. */
+ if (PathIsDirectoryA(path))
+ return 0;
+
+ /* Default ACL already restricts access to creator and owner only. */
+ if (!CreateDirectoryA(path, NULL)) {
+ RTE_LOG_WIN32_ERR("CreateDirectoryA(\"%s\", NULL)", path);
+ rte_errno = EINVAL;
+ return -1;
+ }
+ return 0;
+}
+
static int
sync_func(void *arg __rte_unused)
{
@@ -242,6 +327,12 @@ rte_eal_init(int argc, char **argv)
if (fctret < 0)
exit(1);
+ if (eal_trace_init() < 0) {
+ rte_eal_init_alert("Cannot init trace");
+ rte_errno = EFAULT;
+ return -1;
+ }
+
eal_thread_init_master(rte_config.master_lcore);
RTE_LCORE_FOREACH_SLAVE(i) {
@@ -157,6 +157,15 @@ eal_thread_create(pthread_t *thread)
return 0;
}
+int
+rte_thread_getname(pthread_t id, char *name, size_t len)
+{
+ RTE_SET_USED(id);
+ RTE_SET_USED(name);
+ RTE_SET_USED(len);
+ return -ENOTSUP;
+}
+
int
rte_thread_setname(__rte_unused pthread_t id, __rte_unused const char *name)
{
@@ -11,6 +11,9 @@
#include <rte_windows.h>
+#include <shlobj.h>
+#include <shlwapi.h>
+
/**
* Create a map of processors and cores on the system.
*/
@@ -46,15 +46,13 @@ extern "C" {
typedef long long ssize_t;
#ifndef RTE_TOOLCHAIN_GCC
+
static inline int
-asprintf(char **buffer, const char *format, ...)
+vasprintf(char **buffer, const char *format, va_list arg)
{
int size, ret;
- va_list arg;
- va_start(arg, format);
size = vsnprintf(NULL, 0, format, arg);
- va_end(arg);
if (size < 0)
return -1;
size++;
@@ -63,16 +61,37 @@ asprintf(char **buffer, const char *format, ...)
if (*buffer == NULL)
return -1;
- va_start(arg, format);
ret = vsnprintf(*buffer, size, format, arg);
- va_end(arg);
if (ret != size - 1) {
free(*buffer);
return -1;
}
return ret;
}
-#endif /* RTE_TOOLCHAIN_GCC */
+
+static inline int
+asprintf(char **buffer, const char *format, ...)
+{
+ int ret;
+ va_list arg;
+
+ va_start(arg, format);
+ ret = vasprintf(buffer, format, arg);
+ va_end(arg);
+
+ return ret;
+}
+
+#else /* RTE_TOOLCHAIN_GCC */
+
+/* value as in time.h from UCRT */
+#define TIME_UTC 1
+
+struct timespec;
+
+int timespec_get(struct timespec *ts, int base);
+
+#endif /* !RTE_TOOLCHAIN_GCC */
#ifdef __cplusplus
}