[v5,02/11] eal: introduce internal wrappers for file operations

Message ID 20200525003720.6410-3-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Windows basic memory management |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Dmitry Kozlyuk May 25, 2020, 12:37 a.m. UTC
  EAL common code uses file locking and truncation. Introduce
OS-independent wrappers in order to support both Linux/FreeBSD
and Windows:

* eal_file_lock: lock or unlock an open file.
* eal_file_truncate: enforce a given size for an open file.

Wrappers follow POSIX semantics, but interface is not POSIX,
so that it can be made more clean, e.g. by not mixing locking
operation and behaviour on conflict.

Implementation for Linux and FreeBSD is placed in "unix" subdirectory,
which is intended for common code between the two. Files should be named
after the ones from which the code is factored in OS subdirectory.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/common/eal_common_fbarray.c | 21 ++++-----
 lib/librte_eal/common/eal_private.h        | 47 ++++++++++++++++++++
 lib/librte_eal/freebsd/Makefile            |  4 ++
 lib/librte_eal/linux/Makefile              |  4 ++
 lib/librte_eal/meson.build                 |  4 ++
 lib/librte_eal/unix/eal_unix.c             | 51 ++++++++++++++++++++++
 lib/librte_eal/unix/meson.build            |  6 +++
 7 files changed, 124 insertions(+), 13 deletions(-)
 create mode 100644 lib/librte_eal/unix/eal_unix.c
 create mode 100644 lib/librte_eal/unix/meson.build
  

Comments

Thomas Monjalon May 28, 2020, 7:59 a.m. UTC | #1
25/05/2020 02:37, Dmitry Kozlyuk:
> * eal_file_lock: lock or unlock an open file.
> * eal_file_truncate: enforce a given size for an open file.
[...]
> Implementation for Linux and FreeBSD is placed in "unix" subdirectory,
> which is intended for common code between the two. Files should be named
> after the ones from which the code is factored in OS subdirectory.
[...]
>  lib/librte_eal/unix/eal_unix.c             | 51 ++++++++++++++++++++++

Why naming this file eal_unix?
If it's truly global, it should be unix/eal.c
If it's only about file operations, it could be unix/eal_file.c

Please update MAINTAINERS when creating a new file.
All files or directories must be listed in MAINTAINERS,
even if there is no maintainer for them.
  
Dmitry Kozlyuk May 28, 2020, 10:09 a.m. UTC | #2
On Thu, 28 May 2020 09:59:13 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 25/05/2020 02:37, Dmitry Kozlyuk:
> > * eal_file_lock: lock or unlock an open file.
> > * eal_file_truncate: enforce a given size for an open file.  
> [...]
> > Implementation for Linux and FreeBSD is placed in "unix" subdirectory,
> > which is intended for common code between the two. Files should be named
> > after the ones from which the code is factored in OS subdirectory.  
> [...]
> >  lib/librte_eal/unix/eal_unix.c             | 51 ++++++++++++++++++++++  
> 
> Why naming this file eal_unix?
> If it's truly global, it should be unix/eal.c

We've already discussed this: Makefiles are written in such way that files in
common and arch directories must have distinct names for OS-specific ones.
Until Makefiles exist, this is consistent naming with the rest of librte_eal.

> If it's only about file operations, it could be unix/eal_file.c

Good suggestion, given that more file-related functions are expected to be
extracted here in future (e.g. for tracing).

> Please update MAINTAINERS when creating a new file.
> All files or directories must be listed in MAINTAINERS,
> even if there is no maintainer for them.

Will do.
  
Thomas Monjalon May 28, 2020, 11:29 a.m. UTC | #3
28/05/2020 12:09, Dmitry Kozlyuk:
> On Thu, 28 May 2020 09:59:13 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 25/05/2020 02:37, Dmitry Kozlyuk:
> > > * eal_file_lock: lock or unlock an open file.
> > > * eal_file_truncate: enforce a given size for an open file.  
> > [...]
> > > Implementation for Linux and FreeBSD is placed in "unix" subdirectory,
> > > which is intended for common code between the two. Files should be named
> > > after the ones from which the code is factored in OS subdirectory.  
> > [...]
> > >  lib/librte_eal/unix/eal_unix.c             | 51 ++++++++++++++++++++++  
> > 
> > Why naming this file eal_unix?
> > If it's truly global, it should be unix/eal.c
> 
> We've already discussed this: Makefiles are written in such way that files in
> common and arch directories must have distinct names for OS-specific ones.
> Until Makefiles exist, this is consistent naming with the rest of librte_eal.

Oh right.

> > If it's only about file operations, it could be unix/eal_file.c
> 
> Good suggestion, given that more file-related functions are expected to be
> extracted here in future (e.g. for tracing).
> 
> > Please update MAINTAINERS when creating a new file.
> > All files or directories must be listed in MAINTAINERS,
> > even if there is no maintainer for them.
> 
> Will do.

Thanks
  

Patch

diff --git a/lib/librte_eal/common/eal_common_fbarray.c b/lib/librte_eal/common/eal_common_fbarray.c
index 4f8f1af73..cfcab63e9 100644
--- a/lib/librte_eal/common/eal_common_fbarray.c
+++ b/lib/librte_eal/common/eal_common_fbarray.c
@@ -8,8 +8,8 @@ 
 #include <sys/mman.h>
 #include <stdint.h>
 #include <errno.h>
-#include <sys/file.h>
 #include <string.h>
+#include <unistd.h>
 
 #include <rte_common.h>
 #include <rte_log.h>
@@ -85,10 +85,8 @@  resize_and_map(int fd, void *addr, size_t len)
 	char path[PATH_MAX];
 	void *map_addr;
 
-	if (ftruncate(fd, len)) {
+	if (eal_file_truncate(fd, len)) {
 		RTE_LOG(ERR, EAL, "Cannot truncate %s\n", path);
-		/* pass errno up the chain */
-		rte_errno = errno;
 		return -1;
 	}
 
@@ -778,7 +776,8 @@  rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
 					__func__, path, strerror(errno));
 			rte_errno = errno;
 			goto fail;
-		} else if (flock(fd, LOCK_EX | LOCK_NB)) {
+		} else if (eal_file_lock(
+				fd, EAL_FLOCK_EXCLUSIVE, EAL_FLOCK_RETURN)) {
 			RTE_LOG(DEBUG, EAL, "%s(): couldn't lock %s: %s\n",
 					__func__, path, strerror(errno));
 			rte_errno = EBUSY;
@@ -789,10 +788,8 @@  rte_fbarray_init(struct rte_fbarray *arr, const char *name, unsigned int len,
 		 * still attach to it, but no other process could reinitialize
 		 * it.
 		 */
-		if (flock(fd, LOCK_SH | LOCK_NB)) {
-			rte_errno = errno;
+		if (eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN))
 			goto fail;
-		}
 
 		if (resize_and_map(fd, data, mmap_len))
 			goto fail;
@@ -895,10 +892,8 @@  rte_fbarray_attach(struct rte_fbarray *arr)
 	}
 
 	/* lock the file, to let others know we're using it */
-	if (flock(fd, LOCK_SH | LOCK_NB)) {
-		rte_errno = errno;
+	if (eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN))
 		goto fail;
-	}
 
 	if (resize_and_map(fd, data, mmap_len))
 		goto fail;
@@ -1025,7 +1020,7 @@  rte_fbarray_destroy(struct rte_fbarray *arr)
 		 * has been detached by all other processes
 		 */
 		fd = tmp->fd;
-		if (flock(fd, LOCK_EX | LOCK_NB)) {
+		if (eal_file_lock(fd, EAL_FLOCK_EXCLUSIVE, EAL_FLOCK_RETURN)) {
 			RTE_LOG(DEBUG, EAL, "Cannot destroy fbarray - another process is using it\n");
 			rte_errno = EBUSY;
 			ret = -1;
@@ -1042,7 +1037,7 @@  rte_fbarray_destroy(struct rte_fbarray *arr)
 			 * we're still holding an exclusive lock, so drop it to
 			 * shared.
 			 */
-			flock(fd, LOCK_SH | LOCK_NB);
+			eal_file_lock(fd, EAL_FLOCK_SHARED, EAL_FLOCK_RETURN);
 
 			ret = -1;
 			goto out;
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 869ce183a..cef73d6fe 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -420,4 +420,51 @@  eal_malloc_no_trace(const char *type, size_t size, unsigned int align);
 
 void eal_free_no_trace(void *addr);
 
+/** File locking operation. */
+enum eal_flock_op {
+	EAL_FLOCK_SHARED,    /**< Acquire a shared lock. */
+	EAL_FLOCK_EXCLUSIVE, /**< Acquire an exclusive lock. */
+	EAL_FLOCK_UNLOCK     /**< Release a previously taken lock. */
+};
+
+/** Behavior on file locking conflict. */
+enum eal_flock_mode {
+	EAL_FLOCK_WAIT,  /**< Wait until the file gets unlocked to lock it. */
+	EAL_FLOCK_RETURN /**< Return immediately if the file is locked. */
+};
+
+/**
+ * Lock or unlock the file.
+ *
+ * On failure @code rte_errno @endcode is set to the error code
+ * specified by POSIX flock(3) description.
+ *
+ * @param fd
+ *  Opened file descriptor.
+ * @param op
+ *  Operation to perform.
+ * @param mode
+ *  Behavior on conflict.
+ * @return
+ *  0 on success, (-1) on failure.
+ */
+int
+eal_file_lock(int fd, enum eal_flock_op op, enum eal_flock_mode mode);
+
+/**
+ * Truncate or extend the file to the specified size.
+ *
+ * On failure @code rte_errno @endcode is set to the error code
+ * specified by POSIX ftruncate(3) description.
+ *
+ * @param fd
+ *  Opened file descriptor.
+ * @param size
+ *  Desired file size.
+ * @return
+ *  0 on success, (-1) on failure.
+ */
+int
+eal_file_truncate(int fd, ssize_t size);
+
 #endif /* _EAL_PRIVATE_H_ */
diff --git a/lib/librte_eal/freebsd/Makefile b/lib/librte_eal/freebsd/Makefile
index af95386d4..4654ca2b3 100644
--- a/lib/librte_eal/freebsd/Makefile
+++ b/lib/librte_eal/freebsd/Makefile
@@ -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
diff --git a/lib/librte_eal/linux/Makefile b/lib/librte_eal/linux/Makefile
index 48cc34844..4f39d462c 100644
--- a/lib/librte_eal/linux/Makefile
+++ b/lib/librte_eal/linux/Makefile
@@ -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
diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
index e301f4558..8d492897d 100644
--- a/lib/librte_eal/meson.build
+++ b/lib/librte_eal/meson.build
@@ -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)
 
diff --git a/lib/librte_eal/unix/eal_unix.c b/lib/librte_eal/unix/eal_unix.c
new file mode 100644
index 000000000..b9c57ef18
--- /dev/null
+++ b/lib/librte_eal/unix/eal_unix.c
@@ -0,0 +1,51 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Dmitry Kozlyuk
+ */
+
+#include <sys/file.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include <rte_errno.h>
+
+#include "eal_private.h"
+
+int
+eal_file_truncate(int fd, ssize_t size)
+{
+	int ret;
+
+	ret = ftruncate(fd, size);
+	if (ret)
+		rte_errno = errno;
+
+	return ret;
+}
+
+int
+eal_file_lock(int fd, enum eal_flock_op op, enum eal_flock_mode mode)
+{
+	int sys_flags = 0;
+	int ret;
+
+	if (mode == EAL_FLOCK_RETURN)
+		sys_flags |= LOCK_NB;
+
+	switch (op) {
+	case EAL_FLOCK_EXCLUSIVE:
+		sys_flags |= LOCK_EX;
+		break;
+	case EAL_FLOCK_SHARED:
+		sys_flags |= LOCK_SH;
+		break;
+	case EAL_FLOCK_UNLOCK:
+		sys_flags |= LOCK_UN;
+		break;
+	}
+
+	ret = flock(fd, sys_flags);
+	if (ret)
+		rte_errno = errno;
+
+	return ret;
+}
diff --git a/lib/librte_eal/unix/meson.build b/lib/librte_eal/unix/meson.build
new file mode 100644
index 000000000..cfa1b4ef9
--- /dev/null
+++ b/lib/librte_eal/unix/meson.build
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2020 Dmitry Kozlyuk
+
+sources += files(
+	'eal_unix.c',
+)