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