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 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to