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 <[email protected]> > >>>> > >>>>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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
