[dpdk-dev] [PATCH] test: fix process dup fd close

[dpdk-dev] [PATCH] test: fix process dup fd close

[kkanas]

From: Krzysztof Kanas <kkanas@marvell.com>

process_dup was intending to close it's own fd's but failed to do so

Fixes: af75078fece3 ("first public release")

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
Reviewed-on: https://sj1git1.cavium.com/14906
Reviewed-by: Amit Gupta <agupta3@marvell.com>
Reviewed-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
Tested-by: Jerin Jacob Kollanukkaran <jerinj@marvell.com>
---
 app/test/process.h | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 128ce41219a1..2a6428f104e1 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -11,6 +11,7 @@
 #include <stdlib.h> /* NULL */
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
+#include <dirent.h>
 #include <sys/wait.h>
 
 #include <rte_string_fns.h> /* strlcpy */
@@ -40,8 +41,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
 	char *argv_cpy[numargs + 1];
-	int i, fd, status;
+	int i, fd, fdir, status;
+	struct dirent *dirent = NULL;
+	const char *procdir = "/proc/self/fd/";
 	char path[32];
+	char *endptr;
+	DIR *dir = NULL;
 #ifdef RTE_LIBRTE_PDUMP
 	pthread_t thread;
 #endif
@@ -58,11 +63,36 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 
 		/* close all open file descriptors, check /proc/self/fd to only
 		 * call close on open fds. Exclude fds 0, 1 and 2*/
-		for (fd = getdtablesize(); fd > 2; fd-- ) {
-			snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
-			if (access(path, F_OK) == 0)
-				close(fd);
+		dir = opendir(procdir);
+
+		if (dir == NULL) {
+			rte_panic("Error opening %s: %s\n", procdir,
+					strerror(errno));
+		}
+
+		fdir = dirfd(dir);
+		if (fdir < 0) {
+			status = errno;
+			closedir(dir);
+			rte_panic("Error %d obtaining fd for dir %s: %s\n",
+					fdir, procdir, strerror(status));
 		}
+
+		while ((dirent = readdir(dir)) != NULL) {
+			errno = 0;
+			fd = strtol(dirent->d_name, &endptr, 10);
+			if (errno != 0 || dirent->d_name == endptr) {
+				printf("Error converint name fd %d %s:\n",
+					fd, dirent->d_name);
+				continue;
+			}
+
+			if (fd == fdir || fd <= 2)
+				continue;
+
+			close(fd);
+		}
+
 		printf("Running binary with argv[]:");
 		for (i = 0; i < num; i++)
 			printf("'%s' ", argv_cpy[i]);
-- 
2.21.0

[dpdk-dev] [PATCH v2] test: fix process dup fd close

[kkanas]

From: Krzysztof Kanas <kkanas@marvell.com>

process_dup was intending to close it's own fd's but failed to do so

Fixes: af75078fece3 ("first public release")

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
---
v2:
* remove unnecessary commit msg information

 app/test/process.h | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 128ce41219a1..2a6428f104e1 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -11,6 +11,7 @@
 #include <stdlib.h> /* NULL */
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
+#include <dirent.h>
 #include <sys/wait.h>
 
 #include <rte_string_fns.h> /* strlcpy */
@@ -40,8 +41,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
 	char *argv_cpy[numargs + 1];
-	int i, fd, status;
+	int i, fd, fdir, status;
+	struct dirent *dirent = NULL;
+	const char *procdir = "/proc/self/fd/";
 	char path[32];
+	char *endptr;
+	DIR *dir = NULL;
 #ifdef RTE_LIBRTE_PDUMP
 	pthread_t thread;
 #endif
@@ -58,11 +63,36 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 
 		/* close all open file descriptors, check /proc/self/fd to only
 		 * call close on open fds. Exclude fds 0, 1 and 2*/
-		for (fd = getdtablesize(); fd > 2; fd-- ) {
-			snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
-			if (access(path, F_OK) == 0)
-				close(fd);
+		dir = opendir(procdir);
+
+		if (dir == NULL) {
+			rte_panic("Error opening %s: %s\n", procdir,
+					strerror(errno));
+		}
+
+		fdir = dirfd(dir);
+		if (fdir < 0) {
+			status = errno;
+			closedir(dir);
+			rte_panic("Error %d obtaining fd for dir %s: %s\n",
+					fdir, procdir, strerror(status));
 		}
+
+		while ((dirent = readdir(dir)) != NULL) {
+			errno = 0;
+			fd = strtol(dirent->d_name, &endptr, 10);
+			if (errno != 0 || dirent->d_name == endptr) {
+				printf("Error converint name fd %d %s:\n",
+					fd, dirent->d_name);
+				continue;
+			}
+
+			if (fd == fdir || fd <= 2)
+				continue;
+
+			close(fd);
+		}
+
 		printf("Running binary with argv[]:");
 		for (i = 0; i < num; i++)
 			printf("'%s' ", argv_cpy[i]);
-- 
2.21.0

Re: [dpdk-dev] [PATCH v2] test: fix process dup fd close

[Krzysztof Kanas]

Ping..

On 19-09-02 11:49, kkanas@marvell.com wrote:
> From: Krzysztof Kanas <kkanas@marvell.com>
> 
> process_dup was intending to close it's own fd's but failed to do so
> 
> Fixes: af75078fece3 ("first public release")
> 
> Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
> ---
> v2:
> * remove unnecessary commit msg information
> 
>  app/test/process.h | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h
> index 128ce41219a1..2a6428f104e1 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -11,6 +11,7 @@
>  #include <stdlib.h> /* NULL */
>  #include <string.h> /* strerror */
>  #include <unistd.h> /* readlink */
> +#include <dirent.h>
>  #include <sys/wait.h>
>  
>  #include <rte_string_fns.h> /* strlcpy */
> @@ -40,8 +41,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  {
>  	int num;
>  	char *argv_cpy[numargs + 1];
> -	int i, fd, status;
> +	int i, fd, fdir, status;
> +	struct dirent *dirent = NULL;
> +	const char *procdir = "/proc/self/fd/";
>  	char path[32];
> +	char *endptr;
> +	DIR *dir = NULL;
>  #ifdef RTE_LIBRTE_PDUMP
>  	pthread_t thread;
>  #endif
> @@ -58,11 +63,36 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  
>  		/* close all open file descriptors, check /proc/self/fd to only
>  		 * call close on open fds. Exclude fds 0, 1 and 2*/
> -		for (fd = getdtablesize(); fd > 2; fd-- ) {
> -			snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
> -			if (access(path, F_OK) == 0)
> -				close(fd);
> +		dir = opendir(procdir);
> +
> +		if (dir == NULL) {
> +			rte_panic("Error opening %s: %s\n", procdir,
> +					strerror(errno));
> +		}
> +
> +		fdir = dirfd(dir);
> +		if (fdir < 0) {
> +			status = errno;
> +			closedir(dir);
> +			rte_panic("Error %d obtaining fd for dir %s: %s\n",
> +					fdir, procdir, strerror(status));
>  		}
> +
> +		while ((dirent = readdir(dir)) != NULL) {
> +			errno = 0;
> +			fd = strtol(dirent->d_name, &endptr, 10);
> +			if (errno != 0 || dirent->d_name == endptr) {
> +				printf("Error converint name fd %d %s:\n",
> +					fd, dirent->d_name);
> +				continue;
> +			}
> +
> +			if (fd == fdir || fd <= 2)
> +				continue;
> +
> +			close(fd);
> +		}
> +
>  		printf("Running binary with argv[]:");
>  		for (i = 0; i < num; i++)
>  			printf("'%s' ", argv_cpy[i]);
> -- 
> 2.21.0
> 

-- 
-
Regards,
Krzysztof(Chris) Kanas

Re: [dpdk-dev] [PATCH v2] test: fix process dup fd close

[David Marchand]

On Mon, Sep 2, 2019 at 11:50 AM <kkanas@marvell.com> wrote:
>
> From: Krzysztof Kanas <kkanas@marvell.com>
>
> process_dup was intending to close it's own fd's but failed to do so

A bit hard to digest, what is the problem that you want to fix?

Thanks.


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v2] test: fix process dup fd close

[Krzysztof Kanas]

On 19-10-30 10:06, David Marchand wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On Mon, Sep 2, 2019 at 11:50 AM <kkanas@marvell.com> wrote:
> >
> > From: Krzysztof Kanas <kkanas@marvell.com>
> >
> > process_dup was intending to close it's own fd's but failed to do so
> 
> A bit hard to digest, what is the problem that you want to fix?
> 
> Thanks.

I don't recall the exact test name but I think it was test_eal_flags 
that is included in the meson test suite for fast tests.
This test was timing out on ARM64.

Using strace (-f -c IIRC) showed that the process is spending most of 
the time in access syscall trying to close not existing fd's.

-               for (fd = getdtablesize(); fd > 2; fd-- ) {
-                       snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
-                       if (access(path, F_OK) == 0)
-                               close(fd);

So the simplest way to do so is to close only the opened fd's not
blindly try every possible one.

Also I think the code was wrong in another way as it tried to iterate 
over /proc/ *exe* /fd/%d/ when it should iterate over /proc/ *self* 
/fd/, so either the comment bove the for loop was wrong or the code was 
incorrect.


> 
> 
> -- 
> David Marchand
> 

-- 
-
Regards,
Krzysztof(Chris) Kanas

Re: [dpdk-dev] [EXT] Re: [PATCH v2] test: fix process dup fd close

[David Marchand]

On Mon, Nov 4, 2019 at 8:52 AM Krzysztof Kanas <kkanas@marvell.com> wrote:
>
> On 19-10-30 10:06, David Marchand wrote:
> > External Email
> >
> > ----------------------------------------------------------------------
> > On Mon, Sep 2, 2019 at 11:50 AM <kkanas@marvell.com> wrote:
> > >
> > > From: Krzysztof Kanas <kkanas@marvell.com>
> > >
> > > process_dup was intending to close it's own fd's but failed to do so
> >
> > A bit hard to digest, what is the problem that you want to fix?
> >
> > Thanks.
>
> I don't recall the exact test name but I think it was test_eal_flags
> that is included in the meson test suite for fast tests.
> This test was timing out on ARM64.
>
> Using strace (-f -c IIRC) showed that the process is spending most of
> the time in access syscall trying to close not existing fd's.
>
> -               for (fd = getdtablesize(); fd > 2; fd-- ) {
> -                       snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
> -                       if (access(path, F_OK) == 0)
> -                               close(fd);
>
> So the simplest way to do so is to close only the opened fd's not
> blindly try every possible one.
>
> Also I think the code was wrong in another way as it tried to iterate
> over /proc/ *exe* /fd/%d/ when it should iterate over /proc/ *self*
> /fd/, so either the comment bove the for loop was wrong or the code was
> incorrect.

Thanks.
I agree that this loop is terrible.
I have some comments on the patch.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] test: fix process dup fd close

[David Marchand]

On Mon, Sep 2, 2019 at 11:50 AM <kkanas@marvell.com> wrote:
>
> From: Krzysztof Kanas <kkanas@marvell.com>
>
> process_dup was intending to close it's own fd's but failed to do so

Please, explain how you caught the issue and what the impact was on your system.

>
> Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

>
> Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
> ---
> v2:
> * remove unnecessary commit msg information
>
>  app/test/process.h | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/app/test/process.h b/app/test/process.h
> index 128ce41219a1..2a6428f104e1 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -11,6 +11,7 @@
>  #include <stdlib.h> /* NULL */
>  #include <string.h> /* strerror */
>  #include <unistd.h> /* readlink */
> +#include <dirent.h>
>  #include <sys/wait.h>
>
>  #include <rte_string_fns.h> /* strlcpy */
> @@ -40,8 +41,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>  {
>         int num;
>         char *argv_cpy[numargs + 1];
> -       int i, fd, status;
> +       int i, fd, fdir, status;
> +       struct dirent *dirent = NULL;

No need to initialise to NULL.

> +       const char *procdir = "/proc/self/fd/";

self is a Linux thing.
This won't work on FreeBSD.


>         char path[32];
> +       char *endptr;
> +       DIR *dir = NULL;

Idem, no need to initialize.

>  #ifdef RTE_LIBRTE_PDUMP
>         pthread_t thread;
>  #endif
> @@ -58,11 +63,36 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
>
>                 /* close all open file descriptors, check /proc/self/fd to only
>                  * call close on open fds. Exclude fds 0, 1 and 2*/
> -               for (fd = getdtablesize(); fd > 2; fd-- ) {
> -                       snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
> -                       if (access(path, F_OK) == 0)
> -                               close(fd);
> +               dir = opendir(procdir);
> +

Remove empty line.


> +               if (dir == NULL) {
> +                       rte_panic("Error opening %s: %s\n", procdir,
> +                                       strerror(errno));
> +               }
> +
> +               fdir = dirfd(dir);
> +               if (fdir < 0) {
> +                       status = errno;

No need to set status, this (forked) process will panic.


> +                       closedir(dir);
> +                       rte_panic("Error %d obtaining fd for dir %s: %s\n",
> +                                       fdir, procdir, strerror(status));
>                 }
> +
> +               while ((dirent = readdir(dir)) != NULL) {
> +                       errno = 0;
> +                       fd = strtol(dirent->d_name, &endptr, 10);
> +                       if (errno != 0 || dirent->d_name == endptr) {

If you want to validate that the entire string is valid:

if (errno != 0 || endptr[0] != '\0')

> +                               printf("Error converint name fd %d %s:\n",
> +                                       fd, dirent->d_name);

converting


> +                               continue;
> +                       }
> +
> +                       if (fd == fdir || fd <= 2)
> +                               continue;
> +
> +                       close(fd);
> +               }

Missing closedir().

> +
>                 printf("Running binary with argv[]:");
>                 for (i = 0; i < num; i++)
>                         printf("'%s' ", argv_cpy[i]);
> --
> 2.21.0
>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3 0/3] test: fix timeout in flags autotest

[kkanas]

Hi David,

Thanks for review, hopefully this patch will addresses most of the sutff.
Rest I will address here.

> 
> > +       const char *procdir = "/proc/self/fd/";
> 
> self is a Linux thing.
> This won't work on FreeBSD.

IMHO original code didn't worked on FreeBSD as well.
I have created function to adress in third patch

> No need to set status, this (forked) process will panic.
I am using status to save errno as it value can be overwritten on unlikely
event of closedir failure.

---
v3 changes:
  add missing closedir
  fix strtol string validation test
  removed empty line
  remove unecssary initalization
  add check for '.' and '..' to avoid errors in parsing entries
  fix commit message
  add FreeBSD function for closing files
v2 changes:
  fix commit message

Krzysztof Kanas (3):
  test: fix timeout in flags autotest
  test: move close files to separate function
  test: fix FreeBSD file closing function

 process.h |  183 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 138 insertions(+), 45 deletions(-)

[dpdk-dev] [PATCH v3 1/3] test: fix timeout in flags autotest

[kkanas]

From: Krzysztof Kanas <kkanas@marvell.com>

Test eal_flags_autotest times out on 64 ARM due to excessive calls to
'access' system call. While at it fix process_dup logic so it tests
correct file path for opened fd's.

Fixes: af75078fece3 ("first public release")
Cc: intel.com

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
---
 app/test/process.h | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 128ce41219a1..298ba288087a 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -11,6 +11,7 @@
 #include <stdlib.h> /* NULL */
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
+#include <dirent.h>
 #include <sys/wait.h>
 
 #include <rte_string_fns.h> /* strlcpy */
@@ -40,8 +41,12 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
 	char *argv_cpy[numargs + 1];
-	int i, fd, status;
+	int i, fd, fdir, status;
+	struct dirent *dirent;
+	const char *procdir = "/proc/self/fd/";
 	char path[32];
+	char *endptr;
+	DIR *dir;
 #ifdef RTE_LIBRTE_PDUMP
 	pthread_t thread;
 #endif
@@ -58,11 +63,39 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 
 		/* close all open file descriptors, check /proc/self/fd to only
 		 * call close on open fds. Exclude fds 0, 1 and 2*/
-		for (fd = getdtablesize(); fd > 2; fd-- ) {
-			snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
-			if (access(path, F_OK) == 0)
-				close(fd);
+		dir = opendir(procdir);
+		if (dir == NULL) {
+			rte_panic("Error opening %s: %s\n", procdir,
+					strerror(errno));
 		}
+
+		fdir = dirfd(dir);
+		if (fdir < 0) {
+			status = errno;
+			closedir(dir);
+			rte_panic("Error %d obtaining fd for dir %s: %s\n",
+					fdir, procdir, strerror(status));
+		}
+
+		while ((dirent = readdir(dir)) != NULL) {
+			if (!strcmp(".", dirent->d_name) ||
+			    !strcmp("..", dirent->d_name))
+				continue;
+
+			errno = 0;
+			fd = strtol(dirent->d_name, &endptr, 10);
+			if (errno != 0 || endptr[0] != '\0') {
+				printf("Error converint name fd %d %s:\n",
+					fd, dirent->d_name);
+				continue;
+			}
+			if (fd == fdir || fd <= 2)
+				continue;
+
+			close(fd);
+		}
+		closedir(dir);
+
 		printf("Running binary with argv[]:");
 		for (i = 0; i < num; i++)
 			printf("'%s' ", argv_cpy[i]);
-- 
2.21.0

[dpdk-dev] [PATCH v3 2/3] test: move close files to separate function

[kkanas]

From: Krzysztof Kanas <kkanas@marvell.com>

As iterating over opened files is different on Linux and FreeBSD move
to separate function.

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
---
 app/test/process.h | 89 ++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 39 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 298ba288087a..34afac5a1f39 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -30,6 +30,54 @@ extern void *send_pkts(void *empty);
 extern uint16_t flag_for_send_pkts;
 #endif
 
+
+/* close all open file descriptors, check /proc/self/fd to only
+ * call close on open fds. Exclude fds 0, 1 and 2
+ */
+#ifdef RTE_EXEC_ENV_LINUX
+static inline void
+close_files(void)
+{
+	const char *procdir = "/proc/self/fd/";
+	struct dirent *dirent;
+	int fd, fdir, status;
+	char *endptr;
+	DIR *dir;
+
+	dir = opendir(procdir);
+	if (dir == NULL) {
+		rte_panic("Error opening %s: %s\n", procdir,
+				strerror(errno));
+	}
+
+	fdir = dirfd(dir);
+	if (fdir < 0) {
+		status = errno;
+		closedir(dir);
+		rte_panic("Error %d obtaining fd for dir %s: %s\n",
+				fdir, procdir, strerror(status));
+	}
+
+	while ((dirent = readdir(dir)) != NULL) {
+		if (!strcmp(".", dirent->d_name) ||
+			!strcmp("..", dirent->d_name))
+			continue;
+
+		errno = 0;
+		fd = strtol(dirent->d_name, &endptr, 10);
+		if (errno != 0 || endptr[0] != '\0') {
+			printf("Error converint name fd %d %s:\n",
+				fd, dirent->d_name);
+			continue;
+		}
+		if (fd == fdir || fd <= 2)
+			continue;
+
+		close(fd);
+	}
+	closedir(dir);
+}
+#endif
 /*
  * launches a second copy of the test process using the given argv parameters,
  * which should include argv[0] as the process name. To identify in the
@@ -41,12 +89,8 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
 	char *argv_cpy[numargs + 1];
-	int i, fd, fdir, status;
-	struct dirent *dirent;
-	const char *procdir = "/proc/self/fd/";
+	int i, status;
 	char path[32];
-	char *endptr;
-	DIR *dir;
 #ifdef RTE_LIBRTE_PDUMP
 	pthread_t thread;
 #endif
@@ -61,40 +105,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 		argv_cpy[i] = NULL;
 		num = numargs;
 
-		/* close all open file descriptors, check /proc/self/fd to only
-		 * call close on open fds. Exclude fds 0, 1 and 2*/
-		dir = opendir(procdir);
-		if (dir == NULL) {
-			rte_panic("Error opening %s: %s\n", procdir,
-					strerror(errno));
-		}
-
-		fdir = dirfd(dir);
-		if (fdir < 0) {
-			status = errno;
-			closedir(dir);
-			rte_panic("Error %d obtaining fd for dir %s: %s\n",
-					fdir, procdir, strerror(status));
-		}
-
-		while ((dirent = readdir(dir)) != NULL) {
-			if (!strcmp(".", dirent->d_name) ||
-			    !strcmp("..", dirent->d_name))
-				continue;
-
-			errno = 0;
-			fd = strtol(dirent->d_name, &endptr, 10);
-			if (errno != 0 || endptr[0] != '\0') {
-				printf("Error converint name fd %d %s:\n",
-					fd, dirent->d_name);
-				continue;
-			}
-			if (fd == fdir || fd <= 2)
-				continue;
-
-			close(fd);
-		}
-		closedir(dir);
+		close_files();
 
 		printf("Running binary with argv[]:");
 		for (i = 0; i < num; i++)
-- 
2.21.0

[dpdk-dev] [PATCH v3 3/3] test: fix FreeBSD file closing function

[kkanas]

From: Krzysztof Kanas <kkanas@marvell.com>

The FreeBSD was iterating over non existing procfs entries, where sysctl
could give same information.

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
---
 app/test/process.h | 51 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/app/test/process.h b/app/test/process.h
index 34afac5a1f39..77bf11316355 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -16,6 +16,13 @@
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#ifdef RTE_EXEC_ENV_FREEBSD
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/sysctl.h>
+#include <sys/user.h>
+#endif
+
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -34,7 +41,49 @@ extern uint16_t flag_for_send_pkts;
 /* close all open file descriptors, check /proc/self/fd to only
  * call close on open fds. Exclude fds 0, 1 and 2
  */
-#ifdef RTE_EXEC_ENV_LINUX
+#ifdef RTE_EXEC_ENV_FREEBSD
+static inline void
+close_files(void)
+{
+	int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_FILEDESC };
+	size_t sysctl_len;
+	void *oldp;
+	int ret;
+
+	mib[3] = getpid();
+	ret = sysctl(mib, 4, NULL, &sysctl_len, NULL, 0);
+	if (ret < 0) {
+		rte_panic("Error sysctl failed %d %d %s\n", ret, errno,
+			  strerror(errno));
+		return;
+	}
+	oldp = malloc(sysctl_len);
+	if (!oldp) {
+		rte_panic("Error malloc failed\n");
+		return;
+	}
+	ret = sysctl(mib, 4, oldp, &sysctl_len, NULL, 0);
+	if (ret < 0) {
+		ret = errno;
+		free(oldp);
+		rte_panic("Error sysctl failed %d %d %s\n", ret, errno,
+			  strerror(errno));
+	}
+	char *curr = oldp, *end = (char *)oldp + sysctl_len;
+	struct kinfo_file *kf_info;
+	while (curr < end) {
+		kf_info = (struct kinfo_file *)curr;
+		if (kf_info->kf_fd <= 2) {
+			curr += kf_info->kf_structsize;
+			continue;
+		}
+		close(kf_info->kf_fd);
+		curr += kf_info->kf_structsize;
+	}
+	free(oldp);
+}
+
+#else
 static inline void
 close_files(void)
 {
-- 
2.21.0

Re: [dpdk-dev] [PATCH v3 0/3] test: fix timeout in flags autotest

[David Marchand]

On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
>
> Hi David,
>
> Thanks for review, hopefully this patch will addresses most of the sutff.
> Rest I will address here.
>
> >
> > > +       const char *procdir = "/proc/self/fd/";
> >
> > self is a Linux thing.
> > This won't work on FreeBSD.
>
> IMHO original code didn't worked on FreeBSD as well.
> I have created function to adress in third patch

Indeed...

Well, wait.
Why do we need to close those file descriptors?
FreeBSD has been like this for quite some time.

Can't we just remove this code?
Maybe someone from Intel has an idea of why it was like this?

Adding Bruce.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v3 0/3] test: fix timeout in flags autotest

[David Marchand]

On Fri, Nov 8, 2019 at 12:05 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
> >
> > Hi David,
> >
> > Thanks for review, hopefully this patch will addresses most of the sutff.
> > Rest I will address here.
> >
> > >
> > > > +       const char *procdir = "/proc/self/fd/";
> > >
> > > self is a Linux thing.
> > > This won't work on FreeBSD.
> >
> > IMHO original code didn't worked on FreeBSD as well.
> > I have created function to adress in third patch
>
> Indeed...
>
> Well, wait.
> Why do we need to close those file descriptors?
> FreeBSD has been like this for quite some time.
>
> Can't we just remove this code?
> Maybe someone from Intel has an idea of why it was like this?

Great...

        /* get file for config (fd is always 3) */
        snprintf(path, sizeof(path), "/proc/self/fd/%d", 3);

And this only applies to linux (getting the current file prefix).

-- 
David Marchand

Re: [dpdk-dev] [PATCH v3 0/3] test: fix timeout in flags autotest

[David Marchand]

On Fri, Nov 8, 2019 at 12:05 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
> >
> > Hi David,
> >
> > Thanks for review, hopefully this patch will addresses most of the sutff.
> > Rest I will address here.
> >
> > >
> > > > +       const char *procdir = "/proc/self/fd/";
> > >
> > > self is a Linux thing.
> > > This won't work on FreeBSD.
> >
> > IMHO original code didn't worked on FreeBSD as well.
> > I have created function to adress in third patch
>
> Indeed...
>
> Well, wait.
> Why do we need to close those file descriptors?
> FreeBSD has been like this for quite some time.

We don't know what this is used for.

We know it does not work on FreeBSD, but this does not seem to be a problem.
Introducing something more on FreeBSD is a risk with no actual benefit
at first sight.

Either we take only the first patch under a #ifdef EXEC_ENV_LINUX or
we leave this as is.

Krzystof, is this a problem for you if we postpone and investigate
further for 20.02?


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v3 0/3] test: fix timeout in flags autotest

[Krzysztof Kanas]

On 19-11-08 14:45, David Marchand wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On Fri, Nov 8, 2019 at 12:05 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
> > >
> > > Hi David,
> > >
> > > Thanks for review, hopefully this patch will addresses most of the sutff.
> > > Rest I will address here.
> > >
> > > >
> > > > > +       const char *procdir = "/proc/self/fd/";
> > > >
> > > > self is a Linux thing.
> > > > This won't work on FreeBSD.
> > >
> > > IMHO original code didn't worked on FreeBSD as well.
> > > I have created function to adress in third patch
> >
> > Indeed...
> >
> > Well, wait.
> > Why do we need to close those file descriptors?
> > FreeBSD has been like this for quite some time.
> 
> We don't know what this is used for.
> 
> We know it does not work on FreeBSD, but this does not seem to be a problem.
> Introducing something more on FreeBSD is a risk with no actual benefit
> at first sight.
Ok.

> 
> Either we take only the first patch under a #ifdef EXEC_ENV_LINUX or
> we leave this as is.
I would like have that because ARM64 is main platform for me and this 
makes the test pass, and fixes the difference between the comment and 
the code.
> 
> Krzystof, is this a problem for you if we postpone and investigate
> further for 20.02?
Not a problem.  I would like to have fix for EXEC_ENV_LINUX for now.  
Rest, FreeBSD case and the decisions weather this whole code should be 
deleted can be made later.
> 
> 
> -- 
> David Marchand
> 

-- 
-
Regards,
Krzysztof(Chris) Kanas

Re: [dpdk-dev] [EXT] Re: [PATCH v3 0/3] test: fix timeout in flags autotest

[David Marchand]

On Tue, Nov 12, 2019 at 9:29 AM Krzysztof Kanas <kkanas@marvell.com> wrote:
>
> On 19-11-08 14:45, David Marchand wrote:
> > External Email
> >
> > ----------------------------------------------------------------------
> > On Fri, Nov 8, 2019 at 12:05 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Thanks for review, hopefully this patch will addresses most of the sutff.
> > > > Rest I will address here.
> > > >
> > > > >
> > > > > > +       const char *procdir = "/proc/self/fd/";
> > > > >
> > > > > self is a Linux thing.
> > > > > This won't work on FreeBSD.
> > > >
> > > > IMHO original code didn't worked on FreeBSD as well.
> > > > I have created function to adress in third patch
> > >
> > > Indeed...
> > >
> > > Well, wait.
> > > Why do we need to close those file descriptors?
> > > FreeBSD has been like this for quite some time.
> >
> > We don't know what this is used for.
> >
> > We know it does not work on FreeBSD, but this does not seem to be a problem.
> > Introducing something more on FreeBSD is a risk with no actual benefit
> > at first sight.
> Ok.
>
> >
> > Either we take only the first patch under a #ifdef EXEC_ENV_LINUX or
> > we leave this as is.
> I would like have that because ARM64 is main platform for me and this
> makes the test pass, and fixes the difference between the comment and
> the code.
> >
> > Krzystof, is this a problem for you if we postpone and investigate
> > further for 20.02?
> Not a problem.  I would like to have fix for EXEC_ENV_LINUX for now.
> Rest, FreeBSD case and the decisions weather this whole code should be
> deleted can be made later.

In my tests of the original code, I never saw anything but access -1 ENOENT.
Anyway, just sent a v4, your v3 patch, with just cosmetics and
commitlog updated.


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v3 0/3] test: fix timeout in flags autotest

[Krzysztof Kanas]

On 12/11/2019 21:34, David Marchand wrote:
> On Tue, Nov 12, 2019 at 9:29 AM Krzysztof Kanas <kkanas@marvell.com> wrote:
>> On 19-11-08 14:45, David Marchand wrote:
>>> External Email
>>>
>>> ----------------------------------------------------------------------
>>> On Fri, Nov 8, 2019 at 12:05 PM David Marchand
>>> <david.marchand@redhat.com> wrote:
>>>> On Fri, Nov 8, 2019 at 11:21 AM <kkanas@marvell.com> wrote:
>>>>> Hi David,
>>>>>
>>>>> Thanks for review, hopefully this patch will addresses most of the sutff.
>>>>> Rest I will address here.
>>>>>
>>>>>>> +       const char *procdir = "/proc/self/fd/";
>>>>>> self is a Linux thing.
>>>>>> This won't work on FreeBSD.
>>>>> IMHO original code didn't worked on FreeBSD as well.
>>>>> I have created function to adress in third patch
>>>> Indeed...
>>>>
>>>> Well, wait.
>>>> Why do we need to close those file descriptors?
>>>> FreeBSD has been like this for quite some time.
>>> We don't know what this is used for.
>>>
>>> We know it does not work on FreeBSD, but this does not seem to be a problem.
>>> Introducing something more on FreeBSD is a risk with no actual benefit
>>> at first sight.
>> Ok.
>>
>>> Either we take only the first patch under a #ifdef EXEC_ENV_LINUX or
>>> we leave this as is.
>> I would like have that because ARM64 is main platform for me and this
>> makes the test pass, and fixes the difference between the comment and
>> the code.
>>> Krzystof, is this a problem for you if we postpone and investigate
>>> further for 20.02?
>> Not a problem.  I would like to have fix for EXEC_ENV_LINUX for now.
>> Rest, FreeBSD case and the decisions weather this whole code should be
>> deleted can be made later.
> In my tests of the original code, I never saw anything but access -1 ENOENT.
> Anyway, just sent a v4, your v3 patch, with just cosmetics and
> commitlog updated.

ACK. Tested, the v4 patch works fine.

>
>
-- 
Best Regards
--
Krzysztof Kanas kkanas@marvell.com

[dpdk-dev] [PATCH v4] test: optimise fd closing in forked test process

[David Marchand]

From: Krzysztof Kanas <kkanas@marvell.com>

Caught while investigating timeouts on a ARM64 server.

Stracing a test process running the eal_flags_autotest, we can see that
the fork helper is checking all possible file descriptors from
getdtablesize() to 2, and close the existing ones.
We can do better by inspecting this forked process /proc/self/fd
directory.

Besides, checking file descriptors via /proc/self/fd only makes sense for
Linux. This code was a noop on FreeBSD.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changelog since v3:
- rewrote commit log message,
- restricted the fix to Linux only,

---
 app/test/process.h | 50 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/app/test/process.h b/app/test/process.h
index 128ce41..191d279 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -11,6 +11,7 @@
 #include <stdlib.h> /* NULL */
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
+#include <dirent.h>
 #include <sys/wait.h>
 
 #include <rte_string_fns.h> /* strlcpy */
@@ -40,7 +41,7 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
 	char *argv_cpy[numargs + 1];
-	int i, fd, status;
+	int i, status;
 	char path[32];
 #ifdef RTE_LIBRTE_PDUMP
 	pthread_t thread;
@@ -56,13 +57,50 @@ process_dup(const char *const argv[], int numargs, const char *env_value)
 		argv_cpy[i] = NULL;
 		num = numargs;
 
-		/* close all open file descriptors, check /proc/self/fd to only
-		 * call close on open fds. Exclude fds 0, 1 and 2*/
-		for (fd = getdtablesize(); fd > 2; fd-- ) {
-			snprintf(path, sizeof(path), "/proc/" exe "/fd/%d", fd);
-			if (access(path, F_OK) == 0)
+#ifdef RTE_EXEC_ENV_LINUX
+		{
+			const char *procdir = "/proc/" self "/fd/";
+			struct dirent *dirent;
+			char *endptr;
+			int fd, fdir;
+			DIR *dir;
+
+			/* close all open file descriptors, check /proc/self/fd
+			 * to only call close on open fds. Exclude fds 0, 1 and
+			 * 2
+			 */
+			dir = opendir(procdir);
+			if (dir == NULL) {
+				rte_panic("Error opening %s: %s\n", procdir,
+						strerror(errno));
+			}
+
+			fdir = dirfd(dir);
+			if (fdir < 0) {
+				status = errno;
+				closedir(dir);
+				rte_panic("Error %d obtaining fd for dir %s: %s\n",
+						fdir, procdir,
+						strerror(status));
+			}
+
+			while ((dirent = readdir(dir)) != NULL) {
+				errno = 0;
+				fd = strtol(dirent->d_name, &endptr, 10);
+				if (errno != 0 || endptr[0] != '\0') {
+					printf("Error converting name fd %d %s:\n",
+						fd, dirent->d_name);
+					continue;
+				}
+
+				if (fd == fdir || fd <= 2)
+					continue;
+
 				close(fd);
+			}
+			closedir(dir);
 		}
+#endif
 		printf("Running binary with argv[]:");
 		for (i = 0; i < num; i++)
 			printf("'%s' ", argv_cpy[i]);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v4] test: optimise fd closing in forked test process

[Kevin Traynor]

On 12/11/2019 20:31, David Marchand wrote:
> From: Krzysztof Kanas <kkanas@marvell.com>
> 
> Caught while investigating timeouts on a ARM64 server.
> 
> Stracing a test process running the eal_flags_autotest, we can see that
> the fork helper is checking all possible file descriptors from
> getdtablesize() to 2, and close the existing ones.
> We can do better by inspecting this forked process /proc/self/fd
> directory.
> 
> Besides, checking file descriptors via /proc/self/fd only makes sense for
> Linux. This code was a noop on FreeBSD.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Seems to be an improvement on what is existing.

Acked-by: Kevin Traynor <ktraynor@redhat.com>

[dpdk-dev] [PATCH v4] test: optimise fd closing in forked test process

[David Marchand]

On Thu, Nov 14, 2019 at 8:32 PM Kevin Traynor <ktraynor@redhat.com> wrote:
>
> On 12/11/2019 20:31, David Marchand wrote:
> > From: Krzysztof Kanas <kkanas@marvell.com>
> >
> > Caught while investigating timeouts on a ARM64 server.
> >
> > Stracing a test process running the eal_flags_autotest, we can see that
> > the fork helper is checking all possible file descriptors from
> > getdtablesize() to 2, and close the existing ones.
> > We can do better by inspecting this forked process /proc/self/fd
> > directory.
> >
> > Besides, checking file descriptors via /proc/self/fd only makes sense for
> > Linux. This code was a noop on FreeBSD.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Krzysztof Kanas <kkanas@marvell.com>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

Tested-by: Krzysztof Kanas <kkanas@marvell.com>

> Seems to be an improvement on what is existing.
>
> Acked-by: Kevin Traynor <ktraynor@redhat.com>
>

Applied, thanks.


--
David Marchand