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
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
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
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
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
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
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
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(-)
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
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
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
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
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
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
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
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
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
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
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>
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