* [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) @ 2016-02-13 6:41 Matthew Hall 2016-02-14 20:22 ` Stephen Hemminger 0 siblings, 1 reply; 5+ messages in thread From: Matthew Hall @ 2016-02-13 6:41 UTC (permalink / raw) To: dev Signed-off-by: Matthew Hall <mhall@mhcomputing.net> --- lib/librte_eal/linuxapp/eal/eal_interrupts.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c index 06b26a9..b33ccdb 100644 --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c @@ -800,6 +800,7 @@ static __attribute__((noreturn)) void * eal_intr_thread_main(__rte_unused void *arg) { struct epoll_event ev; + memset(&ev, 0, sizeof(ev)); /* host thread, never break out */ for (;;) { -- 2.5.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) 2016-02-13 6:41 [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) Matthew Hall @ 2016-02-14 20:22 ` Stephen Hemminger 2016-03-17 14:18 ` Thomas Monjalon 0 siblings, 1 reply; 5+ messages in thread From: Stephen Hemminger @ 2016-02-14 20:22 UTC (permalink / raw) To: Matthew Hall; +Cc: dev On Fri, 12 Feb 2016 22:41:18 -0800 Matthew Hall <mhall@mhcomputing.net> wrote: > Signed-off-by: Matthew Hall <mhall@mhcomputing.net> > --- > lib/librte_eal/linuxapp/eal/eal_interrupts.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > index 06b26a9..b33ccdb 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c > +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > @@ -800,6 +800,7 @@ static __attribute__((noreturn)) void * > eal_intr_thread_main(__rte_unused void *arg) > { > struct epoll_event ev; > + memset(&ev, 0, sizeof(ev)); > > /* host thread, never break out */ > for (;;) { I wonder why valgrind is giving this false report. The only place this data structure is used is here, and all fields in epoll_event are correctly set. TAILQ_FOREACH(src, &intr_sources, next) { if (src->callbacks.tqh_first == NULL) continue; /* skip those with no callbacks */ ev.events = EPOLLIN | EPOLLPRI; ev.data.fd = src->intr_handle.fd; /** * add all the uio device file descriptor * into wait list. */ if (epoll_ctl(pfd, EPOLL_CTL_ADD, src->intr_handle.fd, &ev) < 0){ rte_panic("Error adding fd %d epoll_ctl, %s\n", src->intr_handle.fd, strerror(errno)); A better patch would be to move the data structure into the code block used, and get rid of the useless else (rte_panic never returns); and fix the indentation, and use C99 initialization which should make valgrind happier. The moral is don't just slap memsets around diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c index 06b26a9..d53826e 100644 --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c @@ -799,8 +799,6 @@ eal_intr_handle_interrupts(int pfd, unsigned totalfds) static __attribute__((noreturn)) void * eal_intr_thread_main(__rte_unused void *arg) { - struct epoll_event ev; - /* host thread, never break out */ for (;;) { /* build up the epoll fd with all descriptors we are to @@ -834,20 +832,22 @@ eal_intr_thread_main(__rte_unused void *arg) TAILQ_FOREACH(src, &intr_sources, next) { if (src->callbacks.tqh_first == NULL) continue; /* skip those with no callbacks */ - ev.events = EPOLLIN | EPOLLPRI; - ev.data.fd = src->intr_handle.fd; + + struct epoll_event ev = { + .events = EPOLLIN | EPOLLPRI, + .data.fd = src->intr_handle.fd, + }; /** * add all the uio device file descriptor * into wait list. */ if (epoll_ctl(pfd, EPOLL_CTL_ADD, - src->intr_handle.fd, &ev) < 0){ + src->intr_handle.fd, &ev) < 0) rte_panic("Error adding fd %d epoll_ctl, %s\n", src->intr_handle.fd, strerror(errno)); - } - else - numfds++; + + numfds++; } rte_spinlock_unlock(&intr_lock); /* serve the interrupt */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) 2016-02-14 20:22 ` Stephen Hemminger @ 2016-03-17 14:18 ` Thomas Monjalon 2016-03-17 17:19 ` Stephen Hemminger 0 siblings, 1 reply; 5+ messages in thread From: Thomas Monjalon @ 2016-03-17 14:18 UTC (permalink / raw) To: Stephen Hemminger; +Cc: dev, Matthew Hall Hi Stephen, Please, could you turn it into a real patch with your sign-off? Thanks 2016-02-14 12:22, Stephen Hemminger: > A better patch would be to move the data structure into the > code block used, and get rid of the useless else (rte_panic never returns); > and fix the indentation, and use C99 initialization which should make valgrind > happier. > > The moral is don't just slap memsets around > > diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > index 06b26a9..d53826e 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c > +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > @@ -799,8 +799,6 @@ eal_intr_handle_interrupts(int pfd, unsigned totalfds) > static __attribute__((noreturn)) void * > eal_intr_thread_main(__rte_unused void *arg) > { > - struct epoll_event ev; > - > /* host thread, never break out */ > for (;;) { > /* build up the epoll fd with all descriptors we are to > @@ -834,20 +832,22 @@ eal_intr_thread_main(__rte_unused void *arg) > TAILQ_FOREACH(src, &intr_sources, next) { > if (src->callbacks.tqh_first == NULL) > continue; /* skip those with no callbacks */ > - ev.events = EPOLLIN | EPOLLPRI; > - ev.data.fd = src->intr_handle.fd; > + > + struct epoll_event ev = { > + .events = EPOLLIN | EPOLLPRI, > + .data.fd = src->intr_handle.fd, > + }; > > /** > * add all the uio device file descriptor > * into wait list. > */ > if (epoll_ctl(pfd, EPOLL_CTL_ADD, > - src->intr_handle.fd, &ev) < 0){ > + src->intr_handle.fd, &ev) < 0) > rte_panic("Error adding fd %d epoll_ctl, %s\n", > src->intr_handle.fd, strerror(errno)); > - } > - else > - numfds++; > + > + numfds++; > } > rte_spinlock_unlock(&intr_lock); > /* serve the interrupt */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) 2016-03-17 14:18 ` Thomas Monjalon @ 2016-03-17 17:19 ` Stephen Hemminger 2016-03-17 23:00 ` Matthew Hall 0 siblings, 1 reply; 5+ messages in thread From: Stephen Hemminger @ 2016-03-17 17:19 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev, Matthew Hall On Thu, 17 Mar 2016 15:18:15 +0100 Thomas Monjalon <thomas.monjalon@6wind.com> wrote: > Hi Stephen, > > Please, could you turn it into a real patch with your sign-off? > Thanks > > 2016-02-14 12:22, Stephen Hemminger: > > A better patch would be to move the data structure into the > > code block used, and get rid of the useless else (rte_panic never returns); > > and fix the indentation, and use C99 initialization which should make valgrind > > happier. > > > > The moral is don't just slap memsets around > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > index 06b26a9..d53826e 100644 > > --- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > +++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c > > @@ -799,8 +799,6 @@ eal_intr_handle_interrupts(int pfd, unsigned totalfds) > > static __attribute__((noreturn)) void * > > eal_intr_thread_main(__rte_unused void *arg) > > { > > - struct epoll_event ev; > > - > > /* host thread, never break out */ > > for (;;) { > > /* build up the epoll fd with all descriptors we are to > > @@ -834,20 +832,22 @@ eal_intr_thread_main(__rte_unused void *arg) > > TAILQ_FOREACH(src, &intr_sources, next) { > > if (src->callbacks.tqh_first == NULL) > > continue; /* skip those with no callbacks */ > > - ev.events = EPOLLIN | EPOLLPRI; > > - ev.data.fd = src->intr_handle.fd; > > + > > + struct epoll_event ev = { > > + .events = EPOLLIN | EPOLLPRI, > > + .data.fd = src->intr_handle.fd, > > + }; > > > > /** > > * add all the uio device file descriptor > > * into wait list. > > */ > > if (epoll_ctl(pfd, EPOLL_CTL_ADD, > > - src->intr_handle.fd, &ev) < 0){ > > + src->intr_handle.fd, &ev) < 0) > > rte_panic("Error adding fd %d epoll_ctl, %s\n", > > src->intr_handle.fd, strerror(errno)); > > - } > > - else > > - numfds++; > > + > > + numfds++; > > } > > rte_spinlock_unlock(&intr_lock); > > /* serve the interrupt */ > > Sure I thought Matthew would since he reported the issue and had the ability to test it. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) 2016-03-17 17:19 ` Stephen Hemminger @ 2016-03-17 23:00 ` Matthew Hall 0 siblings, 0 replies; 5+ messages in thread From: Matthew Hall @ 2016-03-17 23:00 UTC (permalink / raw) To: Stephen Hemminger; +Cc: Thomas Monjalon, dev On Thu, Mar 17, 2016 at 10:19:24AM -0700, Stephen Hemminger wrote: > > > A better patch would be to move the data structure into the > > > code block used, and get rid of the useless else (rte_panic never returns); > > > and fix the indentation, and use C99 initialization which should make valgrind > > > happier. > > > > > > The moral is don't just slap memsets around Hi guys, As you probably read before, I am a security developer, not a low-level / kernel guy, and my DPDK work is spare time only. So I try to limit the scope of my DPDK patches where possible, to avoid making headaches for the full-time team to the minimum required. I can try to redo or refactor all this unrelated stuff in the code, but I wouldn't be as fast or accurate as you would. Matthew. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-17 23:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-13 6:41 [dpdk-dev] [PATCH] eal_interrupts.c: properly init struct epoll_event (valgrind) Matthew Hall 2016-02-14 20:22 ` Stephen Hemminger 2016-03-17 14:18 ` Thomas Monjalon 2016-03-17 17:19 ` Stephen Hemminger 2016-03-17 23:00 ` Matthew Hall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).