[ovs-dev] [PATCH 0/7] Introduce high resolution sleep support.

Bodireddy, Bhanuprakash bhanuprakash.bodireddy at intel.com
Wed Nov 8 20:54:11 UTC 2017


HI Ben,
>On Wed, Nov 08, 2017 at 04:35:52PM +0000, Bhanuprakash Bodireddy wrote:
>> This patchset introduces high resolution sleep support for linux and
>windows.
>> Also time macros are introduced to replace the numbers with meaningful
>> names.
>
>Thank you very much for the series.
>
>Did you test that the Windows version of the code compiles (e.g. via
>appveyor)?

I cross checked with appveyor and the build was successful. I replied to another thread where we were discussing about the windows implementation.

>
>I would normally squash patch 3 (the Windows version of xnanosleep) into
>patch 2 (the Linux version). 

I couldn't verify the functionality of windows implementation and hence posted it as a separate patch for now.
I will squash it once I receive feedback from Alin.	

 Also, I would normally squash the patches that
>just replace constants by xSEC_PER_ySEC macros into the patch that
>introduced those macros (if there are other changes then I would separate
>those).
Ok.

>
>I am concerned about types.  The xSEC_PER_ySEC macros all use type "long"
>for their constants, but in some cases the code needs to have type "long
>long", for example in many cases when multiplying by one of these macros.
>When the patches replace an LL-suffixed literal by one of the xSEC_PER_ySEC
>macros, this introduces a risk of overflow that was not present before.
>
>I am not certain that the xSEC_PER_ySEC macros clarify things, especially given
>the type issues.  I don't feel strongly about it though.

Yeah I understand your concern here and difficult to test the cases for overflow
with this changes. I will leave it the way it is now.

>
>In the xnanosleep() implementation for Windows, I think that the two calls to
>CloseHandle can be consolidated into one.

Sure.

- Bhanuprakash.


More information about the dev mailing list