[ovs-dev] [PATCH] stopwatch: Explicitly ignore write() return value.

Mark Michelson mmichels at redhat.com
Fri Apr 6 21:13:34 UTC 2018


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 at 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!


More information about the dev mailing list