Ben Pfaff <b...@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 <jpet...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
