| Message ID | 1523034984-103215-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] stopwatch: Explicitly ignore write() return value. | expand |
On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: > In some environments, builds would fail with the following error: > > lib/stopwatch.c: In function ‘stopwatch_exit’: > lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, declared > with attribute warn_unused_result [-Werror=unused-result] > write(stopwatch_pipe[1], &pkt, sizeof pkt); > > This patch explicitly ignores the return value of write(). > > This also fixes some minor coding style issues. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> Obviously I didn't review this as carefully as I should have. That's on me. I apologize. I believe that we already have a proper abstraction for what's going on here: a latch. The latch implementation also hides differences between Unix and Windows (which this inline implementation isn't doing). Would you mind making this use latch.h instead of raw pipes? Would you mind breaking the style issues into a separate commit? Thanks, Ben.
Thanks Justin! Acked-by: Mark Michelson <mmichels@redhat.com> On 04/06/2018 12:16 PM, Justin Pettit wrote: > In some environments, builds would fail with the following error: > > lib/stopwatch.c: In function ‘stopwatch_exit’: > lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, declared > with attribute warn_unused_result [-Werror=unused-result] > write(stopwatch_pipe[1], &pkt, sizeof pkt); > > This patch explicitly ignores the return value of write(). > > This also fixes some minor coding style issues. > > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > lib/stopwatch.c | 25 +++++++++++++------------ > lib/stopwatch.h | 8 +++++--- > 2 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/lib/stopwatch.c b/lib/stopwatch.c > index 80677d000030..a241433838b8 100644 > --- a/lib/stopwatch.c > +++ b/lib/stopwatch.c > @@ -24,6 +24,7 @@ > #include "ovs-thread.h" > #include <unistd.h> > #include "socket-util.h" > +#include "util.h" > > VLOG_DEFINE_THIS_MODULE(stopwatch); > > @@ -236,7 +237,7 @@ add_sample(struct stopwatch *sw, unsigned long long new_sample) > > static bool > stopwatch_get_stats_protected(const char *name, > - struct stopwatch_stats *stats) > + struct stopwatch_stats *stats) > { > struct stopwatch *perf; > > @@ -311,7 +312,7 @@ stopwatch_show_protected(int argc, const char *argv[], struct ds *s) > > static void > stopwatch_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > - const char *argv[], void *ignore OVS_UNUSED) > + const char *argv[], void *ignore OVS_UNUSED) > { > struct ds s = DS_EMPTY_INITIALIZER; > bool success; > @@ -330,15 +331,15 @@ stopwatch_show(struct unixctl_conn *conn, int argc OVS_UNUSED, > > static void > stopwatch_reset(struct unixctl_conn *conn, int argc OVS_UNUSED, > - const char *argv[], void *ignore OVS_UNUSED) > + const char *argv[], void *aux OVS_UNUSED) > { > struct stopwatch_packet pkt = { > .op = OP_RESET, > }; > if (argc > 1) { > - ovs_strlcpy(pkt.name, argv[1], sizeof(pkt.name)); > + ovs_strlcpy(pkt.name, argv[1], sizeof pkt.name); > } > - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); > + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); > unixctl_command_reply(conn, ""); > } > > @@ -406,7 +407,7 @@ stopwatch_thread(void *ign OVS_UNUSED) > > while (!should_exit) { > struct stopwatch_packet pkt; > - while (read(stopwatch_pipe[0], &pkt, sizeof(pkt)) > 0) { > + while (read(stopwatch_pipe[0], &pkt, sizeof pkt) > 0) { > ovs_mutex_lock(&stopwatches_lock); > switch (pkt.op) { > case OP_START_SAMPLE: > @@ -445,7 +446,7 @@ stopwatch_exit(void) > .op = OP_SHUTDOWN, > }; > > - write(stopwatch_pipe[1], &pkt, sizeof pkt); > + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); > xpthread_join(stopwatch_thread_id, NULL); > > /* Process is exiting and we have joined the only > @@ -506,8 +507,8 @@ stopwatch_start(const char *name, unsigned long long ts) > .op = OP_START_SAMPLE, > .time = ts, > }; > - ovs_strlcpy(pkt.name, name, sizeof(pkt.name)); > - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); > + ovs_strlcpy(pkt.name, name, sizeof pkt.name); > + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); > } > > void > @@ -517,8 +518,8 @@ stopwatch_stop(const char *name, unsigned long long ts) > .op = OP_END_SAMPLE, > .time = ts, > }; > - ovs_strlcpy(pkt.name, name, sizeof(pkt.name)); > - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); > + ovs_strlcpy(pkt.name, name, sizeof pkt.name); > + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); > } > > void > @@ -529,7 +530,7 @@ stopwatch_sync(void) > }; > > ovs_mutex_lock(&stopwatches_lock); > - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); > + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); > ovs_mutex_cond_wait(&stopwatches_sync, &stopwatches_lock); > ovs_mutex_unlock(&stopwatches_lock); > } > diff --git a/lib/stopwatch.h b/lib/stopwatch.h > index 6ee7291d9230..91abd64e4c11 100644 > --- a/lib/stopwatch.h > +++ b/lib/stopwatch.h > @@ -26,12 +26,14 @@ enum stopwatch_units { > > struct stopwatch_stats { > unsigned long long count; /* Total number of samples. */ > - enum stopwatch_units unit; /* Unit of following values. */ > + enum stopwatch_units unit; /* Unit of following values. */ > unsigned long long max; /* Maximum value. */ > unsigned long long min; /* Minimum value. */ > double pctl_95; /* 95th percentile. */ > - double ewma_50; /* Exponentially weighted moving average (alpha 0.50). */ > - double ewma_1; /* Exponentially weighted moving average (alpha 0.01). */ > + double ewma_50; /* Exponentially weighted moving average > + (alpha 0.50). */ > + double ewma_1; /* Exponentially weighted moving average > + (alpha 0.01). */ > }; > > /* Create a new stopwatch. >
On 04/06/2018 12:28 PM, Ben Pfaff wrote: > On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: >> In some environments, builds would fail with the following error: >> >> lib/stopwatch.c: In function ‘stopwatch_exit’: >> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, declared >> with attribute warn_unused_result [-Werror=unused-result] >> write(stopwatch_pipe[1], &pkt, sizeof pkt); >> >> This patch explicitly ignores the return value of write(). >> >> This also fixes some minor coding style issues. >> >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > > Obviously I didn't review this as carefully as I should have. That's on > me. I apologize. > > I believe that we already have a proper abstraction for what's going on > here: a latch. The latch implementation also hides differences between > Unix and Windows (which this inline implementation isn't doing). Would > you mind making this use latch.h instead of raw pipes? > > Would you mind breaking the style issues into a separate commit? > > Thanks, > > Ben. Hi Ben, Thanks for pointing out the latch library. I'll post the patch with this change. Mark!
On 04/06/2018 02:18 PM, Mark Michelson wrote: > On 04/06/2018 12:28 PM, Ben Pfaff wrote: >> On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: >>> In some environments, builds would fail with the following error: >>> >>> lib/stopwatch.c: In function ‘stopwatch_exit’: >>> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, >>> declared >>> with attribute warn_unused_result [-Werror=unused-result] >>> write(stopwatch_pipe[1], &pkt, sizeof pkt); >>> >>> This patch explicitly ignores the return value of write(). >>> >>> This also fixes some minor coding style issues. >>> >>> Signed-off-by: Justin Pettit <jpettit@ovn.org> >> >> Obviously I didn't review this as carefully as I should have. That's on >> me. I apologize. >> >> I believe that we already have a proper abstraction for what's going on >> here: a latch. The latch implementation also hides differences between >> Unix and Windows (which this inline implementation isn't doing). Would >> you mind making this use latch.h instead of raw pipes? >> >> Would you mind breaking the style issues into a separate commit? >> >> Thanks, >> >> Ben. > > Hi Ben, > > Thanks for pointing out the latch library. I'll post the patch with this > change. > > Mark! I just had a look, and unfortunately, the current code doesn't translate directly to a latch. The reason is that you can't control the data that is sent to and read from the latch. It's essentially a boolean of "set" or "not set". In the current stopwatch implementation, data packets are written to the pipe and that data is then read by another thread. The actual data is important in this case. You're 100% right that this needs to handle the Windows case. One idea I have is to create a cross-platform "pipe" library. Then the latch library and the stopwatch library can be built on top of the pipe library. What do you think about that? Mark!
On Fri, Apr 06, 2018 at 02:27:16PM -0500, Mark Michelson wrote: > On 04/06/2018 02:18 PM, Mark Michelson wrote: > >On 04/06/2018 12:28 PM, Ben Pfaff wrote: > >>On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: > >>>In some environments, builds would fail with the following error: > >>> > >>> lib/stopwatch.c: In function ‘stopwatch_exit’: > >>> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, > >>>declared > >>> with attribute warn_unused_result [-Werror=unused-result] > >>> write(stopwatch_pipe[1], &pkt, sizeof pkt); > >>> > >>>This patch explicitly ignores the return value of write(). > >>> > >>>This also fixes some minor coding style issues. > >>> > >>>Signed-off-by: Justin Pettit <jpettit@ovn.org> > >> > >>Obviously I didn't review this as carefully as I should have. That's on > >>me. I apologize. > >> > >>I believe that we already have a proper abstraction for what's going on > >>here: a latch. The latch implementation also hides differences between > >>Unix and Windows (which this inline implementation isn't doing). Would > >>you mind making this use latch.h instead of raw pipes? > >> > >>Would you mind breaking the style issues into a separate commit? > >> > >>Thanks, > >> > >>Ben. > > > >Hi Ben, > > > >Thanks for pointing out the latch library. I'll post the patch with this > >change. > > > >Mark! > > I just had a look, and unfortunately, the current code doesn't translate > directly to a latch. The reason is that you can't control the data that is > sent to and read from the latch. It's essentially a boolean of "set" or "not > set". In the current stopwatch implementation, data packets are written to > the pipe and that data is then read by another thread. The actual data is > important in this case. > > You're 100% right that this needs to handle the Windows case. One idea I > have is to create a cross-platform "pipe" library. Then the latch library > and the stopwatch library can be built on top of the pipe library. What do > you think about that? I see that I still didn't read the code carefully. Let's get the code compiling with Justin's patches or an equivalent, and then improve it from there. Possibly a pipe isn't needed. One possibility for passing data between threads is to use a guarded_list from guarded-list.h along with a seq from seq.h.
> On Apr 6, 2018, at 12:27 PM, Mark Michelson <mmichels@redhat.com> wrote: > > I just had a look, and unfortunately, the current code doesn't translate directly to a latch. The reason is that you can't control the data that is sent to and read from the latch. It's essentially a boolean of "set" or "not set". In the current stopwatch implementation, data packets are written to the pipe and that data is then read by another thread. The actual data is important in this case. I noticed the same thing. I spoke with Ben off-line, and I'll push my changes (broken into the fix and the style changes) so that we reduce the chances that people run into at least the issue I had. You'll take care of the bigger change to handle Windows? Thanks! --Justin
On 04/06/2018 02:53 PM, Justin Pettit wrote: > >> On Apr 6, 2018, at 12:27 PM, Mark Michelson <mmichels@redhat.com> wrote: >> >> I just had a look, and unfortunately, the current code doesn't translate directly to a latch. The reason is that you can't control the data that is sent to and read from the latch. It's essentially a boolean of "set" or "not set". In the current stopwatch implementation, data packets are written to the pipe and that data is then read by another thread. The actual data is important in this case. > > I noticed the same thing. I spoke with Ben off-line, and I'll push my changes (broken into the fix and the style changes) so that we reduce the chances that people run into at least the issue I had. You'll take care of the bigger change to handle Windows? > > Thanks! > > --Justin > > Yes, I'll do that. I'll investigate Ben's idea of using a guarded_list and a seq. Mark!
On 04/06/2018 02:50 PM, Ben Pfaff wrote: > On Fri, Apr 06, 2018 at 02:27:16PM -0500, Mark Michelson wrote: >> On 04/06/2018 02:18 PM, Mark Michelson wrote: >>> On 04/06/2018 12:28 PM, Ben Pfaff wrote: >>>> On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: >>>>> In some environments, builds would fail with the following error: >>>>> >>>>> lib/stopwatch.c: In function ‘stopwatch_exit’: >>>>> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, >>>>> declared >>>>> with attribute warn_unused_result [-Werror=unused-result] >>>>> write(stopwatch_pipe[1], &pkt, sizeof pkt); >>>>> >>>>> This patch explicitly ignores the return value of write(). >>>>> >>>>> This also fixes some minor coding style issues. >>>>> >>>>> Signed-off-by: Justin Pettit <jpettit@ovn.org> >>>> >>>> Obviously I didn't review this as carefully as I should have. That's on >>>> me. I apologize. >>>> >>>> I believe that we already have a proper abstraction for what's going on >>>> here: a latch. The latch implementation also hides differences between >>>> Unix and Windows (which this inline implementation isn't doing). Would >>>> you mind making this use latch.h instead of raw pipes? >>>> >>>> Would you mind breaking the style issues into a separate commit? >>>> >>>> Thanks, >>>> >>>> Ben. >>> >>> Hi Ben, >>> >>> Thanks for pointing out the latch library. I'll post the patch with this >>> change. >>> >>> Mark! >> >> I just had a look, and unfortunately, the current code doesn't translate >> directly to a latch. The reason is that you can't control the data that is >> sent to and read from the latch. It's essentially a boolean of "set" or "not >> set". In the current stopwatch implementation, data packets are written to >> the pipe and that data is then read by another thread. The actual data is >> important in this case. >> >> You're 100% right that this needs to handle the Windows case. One idea I >> have is to create a cross-platform "pipe" library. Then the latch library >> and the stopwatch library can be built on top of the pipe library. What do >> you think about that? > > I see that I still didn't read the code carefully. > > Let's get the code compiling with Justin's patches or an equivalent, and > then improve it from there. > > Possibly a pipe isn't needed. One possibility for passing data between > threads is to use a guarded_list from guarded-list.h along with a seq > from seq.h. > This jogged my memory a bit. In version 2 of the stopwatch API change, I used an expanding vector protected by a mutex. This attracted a finding from Aaron Conole. The concerns were 1) Waiting on a global lock can cause the actual real work of the threads using stopwatches to be delayed. 2) Waiting on the lock when ending a sample could lead to a skewed time being reported. Problem 2 is no longer an issue since the stopwatch API now requires the caller to supply the ending timestamp when stopping the stopwatch. However, problem 1 would still be present if I switched to a guarded list and seq, due to the added locking. I switched to the pipe implementation because it eliminated (userspace) locking. So the question here is: what is the best way to go here? Go back to locking, or use a pipe and be sure to support Windows? Mark!
On Fri, Apr 06, 2018 at 04:13:34PM -0500, Mark Michelson wrote: > On 04/06/2018 02:50 PM, Ben Pfaff wrote: > >On Fri, Apr 06, 2018 at 02:27:16PM -0500, Mark Michelson wrote: > >>On 04/06/2018 02:18 PM, Mark Michelson wrote: > >>>On 04/06/2018 12:28 PM, Ben Pfaff wrote: > >>>>On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: > >>>>>In some environments, builds would fail with the following error: > >>>>> > >>>>> lib/stopwatch.c: In function ‘stopwatch_exit’: > >>>>> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, > >>>>>declared > >>>>> with attribute warn_unused_result [-Werror=unused-result] > >>>>> write(stopwatch_pipe[1], &pkt, sizeof pkt); > >>>>> > >>>>>This patch explicitly ignores the return value of write(). > >>>>> > >>>>>This also fixes some minor coding style issues. > >>>>> > >>>>>Signed-off-by: Justin Pettit <jpettit@ovn.org> > >>>> > >>>>Obviously I didn't review this as carefully as I should have. That's on > >>>>me. I apologize. > >>>> > >>>>I believe that we already have a proper abstraction for what's going on > >>>>here: a latch. The latch implementation also hides differences between > >>>>Unix and Windows (which this inline implementation isn't doing). Would > >>>>you mind making this use latch.h instead of raw pipes? > >>>> > >>>>Would you mind breaking the style issues into a separate commit? > >>>> > >>>>Thanks, > >>>> > >>>>Ben. > >>> > >>>Hi Ben, > >>> > >>>Thanks for pointing out the latch library. I'll post the patch with this > >>>change. > >>> > >>>Mark! > >> > >>I just had a look, and unfortunately, the current code doesn't translate > >>directly to a latch. The reason is that you can't control the data that is > >>sent to and read from the latch. It's essentially a boolean of "set" or "not > >>set". In the current stopwatch implementation, data packets are written to > >>the pipe and that data is then read by another thread. The actual data is > >>important in this case. > >> > >>You're 100% right that this needs to handle the Windows case. One idea I > >>have is to create a cross-platform "pipe" library. Then the latch library > >>and the stopwatch library can be built on top of the pipe library. What do > >>you think about that? > > > >I see that I still didn't read the code carefully. > > > >Let's get the code compiling with Justin's patches or an equivalent, and > >then improve it from there. > > > >Possibly a pipe isn't needed. One possibility for passing data between > >threads is to use a guarded_list from guarded-list.h along with a seq > >from seq.h. > > > > This jogged my memory a bit. In version 2 of the stopwatch API change, I > used an expanding vector protected by a mutex. This attracted a finding from > Aaron Conole. > > The concerns were > 1) Waiting on a global lock can cause the actual real work of the threads > using stopwatches to be delayed. > 2) Waiting on the lock when ending a sample could lead to a skewed time > being reported. > > Problem 2 is no longer an issue since the stopwatch API now requires the > caller to supply the ending timestamp when stopping the stopwatch. However, > problem 1 would still be present if I switched to a guarded list and seq, > due to the added locking. I switched to the pipe implementation because it > eliminated (userspace) locking. > > So the question here is: what is the best way to go here? Go back to > locking, or use a pipe and be sure to support Windows? Writing to a pipe can also block of course. Expanding a vector is relatively expensive since it does a memory allocation and copy (in the worst case). None of the guarded_list cases is that expensive, they're just a few pointer operations. So it might be better. If it's a real concern we could invent a cmpxchg based singly linked list I guess.
Ben Pfaff <blp@ovn.org> writes: > On Fri, Apr 06, 2018 at 04:13:34PM -0500, Mark Michelson wrote: >> On 04/06/2018 02:50 PM, Ben Pfaff wrote: >> >On Fri, Apr 06, 2018 at 02:27:16PM -0500, Mark Michelson wrote: >> >>On 04/06/2018 02:18 PM, Mark Michelson wrote: >> >>>On 04/06/2018 12:28 PM, Ben Pfaff wrote: >> >>>>On Fri, Apr 06, 2018 at 10:16:24AM -0700, Justin Pettit wrote: >> >>>>>In some environments, builds would fail with the following error: >> >>>>> >> >>>>> lib/stopwatch.c: In function ‘stopwatch_exit’: >> >>>>> lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, >> >>>>>declared >> >>>>> with attribute warn_unused_result [-Werror=unused-result] >> >>>>> write(stopwatch_pipe[1], &pkt, sizeof pkt); >> >>>>> >> >>>>>This patch explicitly ignores the return value of write(). >> >>>>> >> >>>>>This also fixes some minor coding style issues. >> >>>>> >> >>>>>Signed-off-by: Justin Pettit <jpettit@ovn.org> >> >>>> >> >>>>Obviously I didn't review this as carefully as I should have. That's on >> >>>>me. I apologize. >> >>>> >> >>>>I believe that we already have a proper abstraction for what's going on >> >>>>here: a latch. The latch implementation also hides differences between >> >>>>Unix and Windows (which this inline implementation isn't doing). Would >> >>>>you mind making this use latch.h instead of raw pipes? >> >>>> >> >>>>Would you mind breaking the style issues into a separate commit? >> >>>> >> >>>>Thanks, >> >>>> >> >>>>Ben. >> >>> >> >>>Hi Ben, >> >>> >> >>>Thanks for pointing out the latch library. I'll post the patch with this >> >>>change. >> >>> >> >>>Mark! >> >> >> >>I just had a look, and unfortunately, the current code doesn't translate >> >>directly to a latch. The reason is that you can't control the data that is >> >>sent to and read from the latch. It's essentially a boolean of "set" or "not >> >>set". In the current stopwatch implementation, data packets are written to >> >>the pipe and that data is then read by another thread. The actual data is >> >>important in this case. >> >> >> >>You're 100% right that this needs to handle the Windows case. One idea I >> >>have is to create a cross-platform "pipe" library. Then the latch library >> >>and the stopwatch library can be built on top of the pipe library. What do >> >>you think about that? >> > >> >I see that I still didn't read the code carefully. >> > >> >Let's get the code compiling with Justin's patches or an equivalent, and >> >then improve it from there. >> > >> >Possibly a pipe isn't needed. One possibility for passing data between >> >threads is to use a guarded_list from guarded-list.h along with a seq >> >from seq.h. >> > >> >> This jogged my memory a bit. In version 2 of the stopwatch API change, I >> used an expanding vector protected by a mutex. This attracted a finding from >> Aaron Conole. >> >> The concerns were >> 1) Waiting on a global lock can cause the actual real work of the threads >> using stopwatches to be delayed. >> 2) Waiting on the lock when ending a sample could lead to a skewed time >> being reported. Problem 2 was my biggest concern. I didn't want skew in the samples. Problem 1 is really 'usage scope' issue - if we would put these probes in the fastpath, we'll certainly introduce performance latency & jitter. >> Problem 2 is no longer an issue since the stopwatch API now requires the >> caller to supply the ending timestamp when stopping the stopwatch. However, >> problem 1 would still be present if I switched to a guarded list and seq, >> due to the added locking. I switched to the pipe implementation because it >> eliminated (userspace) locking. >> >> So the question here is: what is the best way to go here? Go back to >> locking, or use a pipe and be sure to support Windows? > > Writing to a pipe can also block of course. +1. > Expanding a vector is relatively expensive since it does a memory > allocation and copy (in the worst case). None of the guarded_list cases > is that expensive, they're just a few pointer operations. So it might > be better. I took a look at guarded list. I think either will work, and both have tradeoffs. The average-cost is probably not worth worrying about, but the worst-case cost could be (if contention happens a lot). I guess pipes might be less prone to worst-case stalls (depending on how often the buffer is cleared), but that's a guess - I have no experience or data to back that up. > If it's a real concern we could invent a cmpxchg based singly linked > list I guess. I think you are referring to something like a lock-free list? If so, agreed. This is the probably most 'consistent' performance (if we do think that's a concern). Likely the message passing fifo doesn't need to even be a linked list (a "lock-free" ring-buffer might a little easier to implement in a provable way). There's lots of ways to skin this cat. I'd probably be more apt to support either the latch/seq approach (because it mirrors the existing implementation), and if we find it isn't good enough - improve it. Anyway, just my $.02
On Fri, Apr 06, 2018 at 06:37:16PM -0400, Aaron Conole wrote: > There's lots of ways to skin this cat. I'd probably be more apt to > support either the latch/seq approach (because it mirrors the existing > implementation), and if we find it isn't good enough - improve it. That's my usual preference. Try the easy way then do it the hard way if it proves not good enough.
diff --git a/lib/stopwatch.c b/lib/stopwatch.c index 80677d000030..a241433838b8 100644 --- a/lib/stopwatch.c +++ b/lib/stopwatch.c @@ -24,6 +24,7 @@ #include "ovs-thread.h" #include <unistd.h> #include "socket-util.h" +#include "util.h" VLOG_DEFINE_THIS_MODULE(stopwatch); @@ -236,7 +237,7 @@ add_sample(struct stopwatch *sw, unsigned long long new_sample) static bool stopwatch_get_stats_protected(const char *name, - struct stopwatch_stats *stats) + struct stopwatch_stats *stats) { struct stopwatch *perf; @@ -311,7 +312,7 @@ stopwatch_show_protected(int argc, const char *argv[], struct ds *s) static void stopwatch_show(struct unixctl_conn *conn, int argc OVS_UNUSED, - const char *argv[], void *ignore OVS_UNUSED) + const char *argv[], void *ignore OVS_UNUSED) { struct ds s = DS_EMPTY_INITIALIZER; bool success; @@ -330,15 +331,15 @@ stopwatch_show(struct unixctl_conn *conn, int argc OVS_UNUSED, static void stopwatch_reset(struct unixctl_conn *conn, int argc OVS_UNUSED, - const char *argv[], void *ignore OVS_UNUSED) + const char *argv[], void *aux OVS_UNUSED) { struct stopwatch_packet pkt = { .op = OP_RESET, }; if (argc > 1) { - ovs_strlcpy(pkt.name, argv[1], sizeof(pkt.name)); + ovs_strlcpy(pkt.name, argv[1], sizeof pkt.name); } - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); unixctl_command_reply(conn, ""); } @@ -406,7 +407,7 @@ stopwatch_thread(void *ign OVS_UNUSED) while (!should_exit) { struct stopwatch_packet pkt; - while (read(stopwatch_pipe[0], &pkt, sizeof(pkt)) > 0) { + while (read(stopwatch_pipe[0], &pkt, sizeof pkt) > 0) { ovs_mutex_lock(&stopwatches_lock); switch (pkt.op) { case OP_START_SAMPLE: @@ -445,7 +446,7 @@ stopwatch_exit(void) .op = OP_SHUTDOWN, }; - write(stopwatch_pipe[1], &pkt, sizeof pkt); + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); xpthread_join(stopwatch_thread_id, NULL); /* Process is exiting and we have joined the only @@ -506,8 +507,8 @@ stopwatch_start(const char *name, unsigned long long ts) .op = OP_START_SAMPLE, .time = ts, }; - ovs_strlcpy(pkt.name, name, sizeof(pkt.name)); - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); + ovs_strlcpy(pkt.name, name, sizeof pkt.name); + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); } void @@ -517,8 +518,8 @@ stopwatch_stop(const char *name, unsigned long long ts) .op = OP_END_SAMPLE, .time = ts, }; - ovs_strlcpy(pkt.name, name, sizeof(pkt.name)); - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); + ovs_strlcpy(pkt.name, name, sizeof pkt.name); + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); } void @@ -529,7 +530,7 @@ stopwatch_sync(void) }; ovs_mutex_lock(&stopwatches_lock); - write(stopwatch_pipe[1], &pkt, sizeof(pkt)); + ignore(write(stopwatch_pipe[1], &pkt, sizeof pkt)); ovs_mutex_cond_wait(&stopwatches_sync, &stopwatches_lock); ovs_mutex_unlock(&stopwatches_lock); } diff --git a/lib/stopwatch.h b/lib/stopwatch.h index 6ee7291d9230..91abd64e4c11 100644 --- a/lib/stopwatch.h +++ b/lib/stopwatch.h @@ -26,12 +26,14 @@ enum stopwatch_units { struct stopwatch_stats { unsigned long long count; /* Total number of samples. */ - enum stopwatch_units unit; /* Unit of following values. */ + enum stopwatch_units unit; /* Unit of following values. */ unsigned long long max; /* Maximum value. */ unsigned long long min; /* Minimum value. */ double pctl_95; /* 95th percentile. */ - double ewma_50; /* Exponentially weighted moving average (alpha 0.50). */ - double ewma_1; /* Exponentially weighted moving average (alpha 0.01). */ + double ewma_50; /* Exponentially weighted moving average + (alpha 0.50). */ + double ewma_1; /* Exponentially weighted moving average + (alpha 0.01). */ }; /* Create a new stopwatch.
In some environments, builds would fail with the following error: lib/stopwatch.c: In function ‘stopwatch_exit’: lib/stopwatch.c:448:5: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result] write(stopwatch_pipe[1], &pkt, sizeof pkt); This patch explicitly ignores the return value of write(). This also fixes some minor coding style issues. Signed-off-by: Justin Pettit <jpettit@ovn.org> --- lib/stopwatch.c | 25 +++++++++++++------------ lib/stopwatch.h | 8 +++++--- 2 files changed, 18 insertions(+), 15 deletions(-)