RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

David Holmes
Bug: https://bugs.openjdk.java.net/browse/JDK-8175817

webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/

On Solaris SIGasync represents what we call the "SR signal" on other
platforms (the event-based suspend/resume signal), and it is hardwired
to SIGJVM2 (a dedicated JVM signal on Solaris 10+).

As this signal is hard-wired and has not been user-definable at runtime
for a very long time, we can do away with the SIGasync field and its
setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
SIGUSR2 as the code currently suggests!). Once this is a build time
constant it simplifies some other code.

We can also stop trying to account for SIGJVM1/2 not existing as they
must exist on the supported Solaris platforms.

We can also stop trying to deal with an older libjsig (the logic for
which has a bug anyway: used <= instead of < in the version check).

Overall just a bit of simplification and cleanup. No functional changes.

Testing: JPRT

Thanks,
David

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

Robbin Ehn
Hi David,

os_solaris.cpp:
3948   DO_SIGNAL_CHECK(ASYNC_SIGNAL);
Maybe move this to line 3938 with the other checks.

Looks good, thanks for fixing.

/Robbin

On 03/16/2017 02:00 PM, David Holmes wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>
> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>
> On Solaris SIGasync represents what we call the "SR signal" on other platforms (the event-based suspend/resume signal), and it is hardwired to SIGJVM2 (a dedicated JVM
> signal on Solaris 10+).
>
> As this signal is hard-wired and has not been user-definable at runtime for a very long time, we can do away with the SIGasync field and its setter and getter, and simply
> define ASYNC_SIGNAL as SIGJVM2 (not SIGUSR2 as the code currently suggests!). Once this is a build time constant it simplifies some other code.
>
> We can also stop trying to account for SIGJVM1/2 not existing as they must exist on the supported Solaris platforms.
>
> We can also stop trying to deal with an older libjsig (the logic for which has a bug anyway: used <= instead of < in the version check).
>
> Overall just a bit of simplification and cleanup. No functional changes.
>
> Testing: JPRT
>
> Thanks,
> David
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

David Holmes
Hi Robbin,

Thanks for looking at this.

On 17/03/2017 1:43 AM, Robbin Ehn wrote:
> Hi David,
>
> os_solaris.cpp:
> 3948   DO_SIGNAL_CHECK(ASYNC_SIGNAL);
> Maybe move this to line 3938 with the other checks.

Sure.

Thanks,
David
-----

> Looks good, thanks for fixing.
>
> /Robbin
>
> On 03/16/2017 02:00 PM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>>
>> On Solaris SIGasync represents what we call the "SR signal" on other
>> platforms (the event-based suspend/resume signal), and it is hardwired
>> to SIGJVM2 (a dedicated JVM
>> signal on Solaris 10+).
>>
>> As this signal is hard-wired and has not been user-definable at
>> runtime for a very long time, we can do away with the SIGasync field
>> and its setter and getter, and simply
>> define ASYNC_SIGNAL as SIGJVM2 (not SIGUSR2 as the code currently
>> suggests!). Once this is a build time constant it simplifies some
>> other code.
>>
>> We can also stop trying to account for SIGJVM1/2 not existing as they
>> must exist on the supported Solaris platforms.
>>
>> We can also stop trying to deal with an older libjsig (the logic for
>> which has a bug anyway: used <= instead of < in the version check).
>>
>> Overall just a bit of simplification and cleanup. No functional changes.
>>
>> Testing: JPRT
>>
>> Thanks,
>> David
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

David Holmes
In reply to this post by David Holmes
Can I get a Review please.

Thanks,
David

On 16/03/2017 11:00 PM, David Holmes wrote:

> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>
> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>
> On Solaris SIGasync represents what we call the "SR signal" on other
> platforms (the event-based suspend/resume signal), and it is hardwired
> to SIGJVM2 (a dedicated JVM signal on Solaris 10+).
>
> As this signal is hard-wired and has not been user-definable at runtime
> for a very long time, we can do away with the SIGasync field and its
> setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
> SIGUSR2 as the code currently suggests!). Once this is a build time
> constant it simplifies some other code.
>
> We can also stop trying to account for SIGJVM1/2 not existing as they
> must exist on the supported Solaris platforms.
>
> We can also stop trying to deal with an older libjsig (the logic for
> which has a bug anyway: used <= instead of < in the version check).
>
> Overall just a bit of simplification and cleanup. No functional changes.
>
> Testing: JPRT
>
> Thanks,
> David
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

Thomas Stüfe-2
Hi David,

I had a look a the changes and think them fine so far.

The stricter libjsig version check means that applications will break now
which did embed the libjsig into their applications by grabbing it from the
VM and shipping it separately, and where the libjslig.so is now outdated.
But I guess this is a remote scenario, and probably also illegal.
http://docs.oracle.com/javase/7/docs/technotes/guides/vm/signal-chaining.html
only documents linking against the libjsig.so from the directory of the
embedded VM.

So we now require the build machine to provide SIGJVM1/2. That is fine and
a good cleanup. Do I understand this right, were we now to run on older
OSes which do not know SIGJVM1, we would probably silently use the SIGRT..
range, right, because 40 falls into this? Or is that no issue, are there
runtime checks preventing us from starting on older releases?

Apart from this, I wonder if we could at some time unite this SR_.. coding.
Handling for suspend/resume looks similar on all the other posix platforms.
But that ties in with whether signal handling in general could be unified
for all posix platforms.

Kind Regards, Thomas

p.s. I like the fact that solaris has a SIGJVM1/2, I did not know that. The
things one can do if one owns the OS :)


On Fri, Mar 17, 2017 at 10:58 PM, David Holmes <[hidden email]>
wrote:

> Can I get a Review please.
>
> Thanks,
> David
>
>
> On 16/03/2017 11:00 PM, David Holmes wrote:
>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>>
>> On Solaris SIGasync represents what we call the "SR signal" on other
>> platforms (the event-based suspend/resume signal), and it is hardwired
>> to SIGJVM2 (a dedicated JVM signal on Solaris 10+).
>>
>> As this signal is hard-wired and has not been user-definable at runtime
>> for a very long time, we can do away with the SIGasync field and its
>> setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
>> SIGUSR2 as the code currently suggests!). Once this is a build time
>> constant it simplifies some other code.
>>
>> We can also stop trying to account for SIGJVM1/2 not existing as they
>> must exist on the supported Solaris platforms.
>>
>> We can also stop trying to deal with an older libjsig (the logic for
>> which has a bug anyway: used <= instead of < in the version check).
>>
>> Overall just a bit of simplification and cleanup. No functional changes.
>>
>> Testing: JPRT
>>
>> Thanks,
>> David
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

Dmitry Samersoff
In reply to this post by David Holmes
David,

Looks good to me.

I'm not sure we need a strict check of libjsig version
at os_solaris.cpp:4044 but it worth to have it.

-Dmitry


On 2017-03-18 00:58, David Holmes wrote:

> Can I get a Review please.
>
> Thanks,
> David
>
> On 16/03/2017 11:00 PM, David Holmes wrote:
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>>
>> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>>
>> On Solaris SIGasync represents what we call the "SR signal" on other
>> platforms (the event-based suspend/resume signal), and it is hardwired
>> to SIGJVM2 (a dedicated JVM signal on Solaris 10+).
>>
>> As this signal is hard-wired and has not been user-definable at runtime
>> for a very long time, we can do away with the SIGasync field and its
>> setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
>> SIGUSR2 as the code currently suggests!). Once this is a build time
>> constant it simplifies some other code.
>>
>> We can also stop trying to account for SIGJVM1/2 not existing as they
>> must exist on the supported Solaris platforms.
>>
>> We can also stop trying to deal with an older libjsig (the logic for
>> which has a bug anyway: used <= instead of < in the version check).
>>
>> Overall just a bit of simplification and cleanup. No functional changes.
>>
>> Testing: JPRT
>>
>> Thanks,
>> David
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

David Holmes
In reply to this post by Thomas Stüfe-2
Hi Thomas,

On 18/03/2017 5:47 PM, Thomas Stüfe wrote:
> Hi David,
>
> I had a look a the changes and think them fine so far.

Thanks for looking at this.

> The stricter libjsig version check means that applications will break
> now which did embed the libjsig into their applications by grabbing it
> from the VM and shipping it separately, and where the libjslig.so is now
> outdated. But I guess this is a remote scenario, and probably also
> illegal.
> http://docs.oracle.com/javase/7/docs/technotes/guides/vm/signal-chaining.html
> only documents linking against the libjsig.so from the directory of the
> embedded VM.

Right. There was never any expectation that anyone would link libjsig
into their application. But the point is moot anyway as the version
number has never changed since its introduction in 1.4.1.

> So we now require the build machine to provide SIGJVM1/2. That is fine
> and a good cleanup. Do I understand this right, were we now to run on
> older OSes which do not know SIGJVM1, we would probably silently use the
> SIGRT.. range, right, because 40 falls into this? Or is that no issue,
> are there runtime checks preventing us from starting on older releases?

We've required a Solaris 10 minimum build and runtime machine for quite
a while now. I don't know what would happen if you tried to run current
binaries on Solaris 9. By the time 10 is release the minimum platforms
may have moved on even further.

> Apart from this, I wonder if we could at some time unite this SR_..
> coding. Handling for suspend/resume looks similar on all the other posix
> platforms. But that ties in with whether signal handling in general
> could be unified for all posix platforms.

Certainly possible, but pretty low priority.

> Kind Regards, Thomas
>
> p.s. I like the fact that solaris has a SIGJVM1/2, I did not know that.
> The things one can do if one owns the OS :)

Indeed :)

Thank again for looking at this and giving it such scrutiny.

David

>
> On Fri, Mar 17, 2017 at 10:58 PM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Can I get a Review please.
>
>     Thanks,
>     David
>
>
>     On 16/03/2017 11:00 PM, David Holmes wrote:
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>         <https://bugs.openjdk.java.net/browse/JDK-8175817>
>
>         webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>         <http://cr.openjdk.java.net/~dholmes/8175817/webrev/>
>
>         On Solaris SIGasync represents what we call the "SR signal" on other
>         platforms (the event-based suspend/resume signal), and it is
>         hardwired
>         to SIGJVM2 (a dedicated JVM signal on Solaris 10+).
>
>         As this signal is hard-wired and has not been user-definable at
>         runtime
>         for a very long time, we can do away with the SIGasync field and its
>         setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
>         SIGUSR2 as the code currently suggests!). Once this is a build time
>         constant it simplifies some other code.
>
>         We can also stop trying to account for SIGJVM1/2 not existing as
>         they
>         must exist on the supported Solaris platforms.
>
>         We can also stop trying to deal with an older libjsig (the logic for
>         which has a bug anyway: used <= instead of < in the version check).
>
>         Overall just a bit of simplification and cleanup. No functional
>         changes.
>
>         Testing: JPRT
>
>         Thanks,
>         David
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (10) (S) 8175817: Clean up Solaris signal code: SIGUSR2, SIGasync, SIGJVM1/2

David Holmes
In reply to this post by Dmitry Samersoff
Hi Dmitry,

On 18/03/2017 6:03 PM, Dmitry Samersoff wrote:
> David,
>
> Looks good to me.

Thanks for looking at this.

> I'm not sure we need a strict check of libjsig version
> at os_solaris.cpp:4044 but it worth to have it.

Having just explained the versioning situation in reply to Thomas I
realise that this guarantee is pointless: if the get_version function
exists then the only thing it can return is JSIG_VERSION_1_4_1, as that
is the only version we've ever set. I'll change it to an assert just as
a sanity check.

Thanks,
David

> -Dmitry
>
>
> On 2017-03-18 00:58, David Holmes wrote:
>> Can I get a Review please.
>>
>> Thanks,
>> David
>>
>> On 16/03/2017 11:00 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8175817
>>>
>>> webrev: http://cr.openjdk.java.net/~dholmes/8175817/webrev/
>>>
>>> On Solaris SIGasync represents what we call the "SR signal" on other
>>> platforms (the event-based suspend/resume signal), and it is hardwired
>>> to SIGJVM2 (a dedicated JVM signal on Solaris 10+).
>>>
>>> As this signal is hard-wired and has not been user-definable at runtime
>>> for a very long time, we can do away with the SIGasync field and its
>>> setter and getter, and simply define ASYNC_SIGNAL as SIGJVM2 (not
>>> SIGUSR2 as the code currently suggests!). Once this is a build time
>>> constant it simplifies some other code.
>>>
>>> We can also stop trying to account for SIGJVM1/2 not existing as they
>>> must exist on the supported Solaris platforms.
>>>
>>> We can also stop trying to deal with an older libjsig (the logic for
>>> which has a bug anyway: used <= instead of < in the version check).
>>>
>>> Overall just a bit of simplification and cleanup. No functional changes.
>>>
>>> Testing: JPRT
>>>
>>> Thanks,
>>> David
>>>
>
>
Loading...