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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
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

Thanks,
Coleen
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Coleen,

Overall this seems okay but a few comments ...

On 9/01/2018 8:34 AM, [hidden email] 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.

---

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.

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

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.

---

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>
   ...

---

Thanks,
David

> Thanks,
> Coleen
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


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, [hidden email] 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.

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

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by coleen.phillimore

I didn't make the change for 8130038 for AIX because it was slightly
different.   The AIX code still uses semaphores directly, so this would
need another RFE.

Thanks,
Coleen

On 1/8/18 5:34 PM, [hidden email] 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
>
> Thanks,
> Coleen

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by coleen.phillimore


On 1/9/18 8:14 AM, [hidden email] 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, [hidden email] 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.

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
>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Coleen,

On 10/01/2018 3:17 AM, [hidden email] wrote:

> On 1/9/18 8:14 AM, [hidden email] 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, [hidden email] 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
>>
>
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 1/10/18 12:11 AM, David Holmes wrote:

> Hi Coleen,
>
> On 10/01/2018 3:17 AM, [hidden email] wrote:
>> On 1/9/18 8:14 AM, [hidden email] 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, [hidden email] 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!

Right, there is no unpackTime on BSD so I removed this for BSD !APPLE
semaphores, since nothing that we have compiles this, and anything that
needs it has more work to do.

Yes, please file an RFE.  I might get to it but there was a complication
for posix because it also needs to get this clock_gettime function: 
os::Linux::clock_gettime(CLOCK_REALTIME, &ts);  We're never done, but
hopefully my change is a start.

Thanks,
Coleen

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

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
<trimming>

On 10/01/2018 10:51 PM, [hidden email] wrote:

> On 1/10/18 12:11 AM, David Holmes wrote:
>> 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!
>
> Right, there is no unpackTime on BSD so I removed this for BSD !APPLE
> semaphores, since nothing that we have compiles this, and anything that
> needs it has more work to do.

Plain BSD should use the shared Posix implementation - including the
timespec logic once it is put into shared code.

> Yes, please file an RFE.  I might get to it but there was a complication
> for posix because it also needs to get this clock_gettime function:
> os::Linux::clock_gettime(CLOCK_REALTIME, &ts);  We're never done, but
> hopefully my change is a start.

https://bugs.openjdk.java.net/browse/JDK-8194860

We already handle clock_gettime in os_posix.cpp - just need to expose it
to the semaphore class. Or maybe for JDK 11 we can finally do away with
the dynamic lookup?

Cheers,
David

> Thanks,
> Coleen
>>
>> 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
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Harold David Seigel
In reply to this post by coleen.phillimore
Hi Coleen,

This change looks good.

Thanks, Harold

On 1/10/2018 7:51 AM, [hidden email] wrote:

>
>
> On 1/10/18 12:11 AM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 10/01/2018 3:17 AM, [hidden email] wrote:
>>> On 1/9/18 8:14 AM, [hidden email] 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, [hidden email] 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!
>
> Right, there is no unpackTime on BSD so I removed this for BSD !APPLE
> semaphores, since nothing that we have compiles this, and anything
> that needs it has more work to do.
>
> Yes, please file an RFE.  I might get to it but there was a
> complication for posix because it also needs to get this clock_gettime
> function:  os::Linux::clock_gettime(CLOCK_REALTIME, &ts);  We're never
> done, but hopefully my change is a start.
>
> Thanks,
> Coleen
>>
>> 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
>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

Thanks Harold!
Coleen

On 1/11/18 3:07 PM, harold seigel wrote:

> Hi Coleen,
>
> This change looks good.
>
> Thanks, Harold
>
> On 1/10/2018 7:51 AM, [hidden email] wrote:
>>
>>
>> On 1/10/18 12:11 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 10/01/2018 3:17 AM, [hidden email] wrote:
>>>> On 1/9/18 8:14 AM, [hidden email] 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, [hidden email] 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!
>>
>> Right, there is no unpackTime on BSD so I removed this for BSD !APPLE
>> semaphores, since nothing that we have compiles this, and anything
>> that needs it has more work to do.
>>
>> Yes, please file an RFE.  I might get to it but there was a
>> complication for posix because it also needs to get this
>> clock_gettime function: os::Linux::clock_gettime(CLOCK_REALTIME,
>> &ts);  We're never done, but hopefully my change is a start.
>>
>> Thanks,
>> Coleen
>>>
>>> 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
>>>>>
>>>>
>>
>