RFR (S) 8130039: Move the platform-specific [OS]Semaphore code

[David Holmes]

Hi Coleen,

On 10/01/2018 3:17 AM, coleen.phillimore at oracle.com wrote:
> On 1/9/18 8:14 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 1/8/18 8:56 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Overall this seems okay but a few comments ...
>>>
>>> On 9/01/2018 8:34 AM, coleen.phillimore at oracle.com wrote:
>>>> Also:
>>>> 8130038: Unify the semaphore usage in os_xxx.cpp
>>>>
>>>> Tested with hs-tier1-5.  And 
>>>> open/test/jdk/sun/misc/SunMiscSignalTest.java and closed signal tests.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8130039.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8130039
>>>
>>> With the removal of os::signal_lookup() the parameter to 
>>> check_pending_signals becomes irrelevant and the "false" path becomes 
>>> dead code. Not sure if it is worth deleting that.
>>
>> Yes, that's true.  I can delete it and the code for the "false" path.
>>>
>>> ---
>>>
>>> os_linux.cpp:
>>>
>>> static struct timespec create_timespec(unsigned int sec, int nsec) {
>>>   struct timespec ts;
>>>   // Semaphore's are always associated with CLOCK_REALTIME
>>>   os::Linux::clock_gettime(CLOCK_REALTIME, &ts);
>>>   // see unpackTime for discussion on overflow checking
>>>
>>> There's no longer a direct connection between create_timespec and the 
>>> fact it is only used with semaphores - so the initial comment is no 
>>> longer appropriate. But if you rename to create_semaphore_timespec 
>>> then that restores the connection.
>>>
>>
>> Ok, I'll rename it.
>>
>>> The final comment is not correct (my bad!) as unpackTime no longer 
>>> exists since I factored out all the PlatformEvent code. It could say:
>>>
>>> // see os_posix.cpp for discussion on overflow checking
>>
>> I see, it's now similar to calc_rel_time in os_posix.cpp.   I'll 
>> change the comment as suggested in case calc_rel_time changes we don't 
>> have to change this.
>>>
>>> but I'm wondering whether the timedwait API should be taking a 
>>> timespec in the first place? If we pass secs and nsecs directly then 
>>> we only need to convert to a timespec inside the low-level 
>>> implementation that needs it ie in semaphore_posix.cpp. I don't 
>>> recall if there was a reason we have to do the timespec conversion in 
>>> a platform specific manner.
>>>
>>
>> I didn't understand the difference between solaris and linux 
>> create_timespec, so I left it in the platform specific code.  The 
>> solaris implementation *does* have unpackTime and the linux one does 
>> not.  Or could I use the linux version in semaphore_posix.cpp ? That 
>> would be better, but I wasn't sure if it was correct.   Let me know. 
>> I'd like to make that change.
> 
> No, looking again, the linux version of create_semaphore_timespec is 
> competely different and I don't think I can unify these at this time.

Well this is quite a mess :( The Solaris unpackTime code is technically 
incorrect if Solaris is using posix semaphores because it reads the time 
from gettimeofday, but the manpage for sem_timedwait clearly states the 
timespec should be from CLOCK_REALTIME. So in actuality what we 
currently have in the Linux code is what the shared POSIX code should 
look like and what Solaris should actually use.

Further, the non-OSX BSD code purports to use PosixSemaphore but it defines:

struct timespec PosixSemaphore::create_timespec(unsigned int sec, int 
nsec) {
   struct timespec ts;
   unpackTime(&ts, false, (sec * NANOSECS_PER_SEC) + nsec);

   return ts;
}

but there is no unpackTime method on BSD!

I think we need a separate RFE to clean up this mess. I'll file one.

Thanks,
David

> thanks,
> Coleen
>>> ---
>>>
>>> semaphore_posix.cpp
>>>
>>>   26 #ifndef __APPLE__
>>>   27 #include "runtime/os.hpp"
>>>   28 // POSIX unamed semaphores are not supported on OS X.
>>>   29 #include "semaphore_posix.hpp"
>>>   30 #include <semaphore.h>
>>>   31 #endif
>>>   32
>>>   33
>>>   34 // POSIX unamed semaphores are not supported on OS X.
>>>   35 #ifndef __APPLE__
>>>
>>> The whole file contents should be ifndef __APPLE__ so this just 
>>> reduces to:
>>>
>>>   // POSIX unamed semaphores are not supported on OS X.
>>>   #ifndef __APPLE__
>>>   #include "runtime/os.hpp"
>>>   #include "semaphore_posix.hpp"
>>>   #include <semaphore.h>
>>>   ...
>>>
>>
>> Ok.  I'll fix that.
>>
>> Thanks,
>> Coleen
>>> ---
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks,
>>>> Coleen
>>
>