[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: RE: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2015-11-17 15:30:56
Message-ID: 4295855A5C1DE049A61835A1887419CC41EB959C () DEWDFEMB12A ! global ! corp ! sap
[Download RAW message or body]
Great, looks good. Thanks a lot!
Best regards,
Goetz.
> -----Original Message-----
> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com]
> Sent: Dienstag, 17. November 2015 14:42
> To: Lindenmaier, Goetz; Thomas Stüfe
> Cc: David Holmes; hotspot-runtime-dev@openjdk.java.net; serviceability-
> dev
> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
>
> Goetz,
>
> Push done.
> Please, take a look at the changeset:
>
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/149cc1f9f1aa
>
> -Dmitry
>
> On 2015-11-17 11:35, Lindenmaier, Goetz wrote:
> > That's great, thanks a lot!
> >
> > Best regards,
> > Goetz.
> >
> >> -----Original Message-----
> >> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com]
> >> Sent: Dienstag, 17. November 2015 09:34
> >> To: Lindenmaier, Goetz; Thomas Stüfe
> >> Cc: David Holmes; hotspot-runtime-dev@openjdk.java.net; serviceability-
> >> dev
> >> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
> >>
> >> Goetz,
> >>
> >> I'll sponsor the push
> >>
> >> -Dmitry.
> >>
> >> On 2015-11-17 10:52, Lindenmaier, Goetz wrote:
> >>> David, Dmitry,
> >>>
> >>> I think this is reviewed now. Could one of you please sponsor this?
> >>> The final webrev, including a full changeset patch, is this:
> >>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-NSIG/webrev.02/
> >>>
> >>> It ran successfully through our nightly tests, tonight.
> >>>
> >>> Best regards,
> >>> Goetz.
> >>>
> >>>> -----Original Message-----
> >>>> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com]
> >>>> Sent: Montag, 16. November 2015 11:53
> >>>> To: Lindenmaier, Goetz; Thomas Stüfe
> >>>> Cc: David Holmes; hotspot-runtime-dev@openjdk.java.net;
> serviceability-
> >>>> dev
> >>>> Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
> >>>>
> >>>> Goetz,
> >>>>
> >>>> Looks good for me.
> >>>>
> >>>> Thank you for a nice cleanup.
> >>>>
> >>>> -Dmitry
> >>>>
> >>>>
> >>>> On 2015-11-16 13:25, Lindenmaier, Goetz wrote:
> >>>>> Hi Thomas,
> >>>>>
> >>>>>
> >>>>>
> >>>>> thanks for looking at this.
> >>>>>
> >>>>>
> >>>>>
> >>>>>> MAX2(SIGSEGV, SIGBUS)
> >>>>>
> >>>>> I really would like to leave the code as-is. This would be a functional
> >>>>>
> >>>>> change, while I only intend to fix issues in this change. Also, as David
> >>>>>
> >>>>> explained, it might break for some os implementations.
> >>>>>
> >>>>>
> >>>>>
> >>>>>> the only way to initialize it is with one of sigemptyset() or sigfillset().
> >>>>>
> >>>>> I added initialization with sigemptyset(). Unfortunately, there is no
> >>>>> static
> >>>>>
> >>>>> initializer for this.
> >>>>>
> >>>>>
> >>>>>
> >>>>>> I would like to see those removed from os::Aix and put into
> os_aix.cpp
> >> at
> >>>> static filescope
> >>>>>
> >>>>> I moved these to static scope on the three oses.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Here is the new webrev:
> >>>>>
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> NSIG/webrev.02/
> >>>>>
> >>>>>
> >>>>>
> >>>>> Best regards,
> >>>>>
> >>>>> Goetz.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> *From:*Thomas Stüfe [mailto:thomas.stuefe@gmail.com]
> >>>>> *Sent:* Freitag, 13. November 2015 10:54
> >>>>> *To:* Lindenmaier, Goetz
> >>>>> *Cc:* Dmitry Samersoff; David Holmes;
> >>>>> hotspot-runtime-dev@openjdk.java.net; serviceability-dev
> >>>>> *Subject:* Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
> >>>>>
> >>>>>
> >>>>>
> >>>>> Hi Goetz,
> >>>>>
> >>>>>
> >>>>>
> >>>>> sorry for not looking at this earlier. This is a nice cleanup. Some
> remarks:
> >>>>>
> >>>>>
> >>>>>
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >>>> NSIG/webrev.01/src/os/aix/vm/os_aix.cpp.udiff.html
> >>>>>
> >>>>>
> >>>>>
> >>>>> + if (sig > MAX2(SIGSEGV, SIGBUS) && // See 4355769.
> >>>>>
> >>>>> + sig < NSIG) { // Must be legal signal and fit
> >>>>> into sigflags[].
> >>>>>
> >>>>>
> >>>>>
> >>>>> I do not like much the MAX2() construct. I would like it better to
> >>>>> explicitly check whether the SR signal is one of the "forbidden" ones
> >>>>> the VM uses.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Maybe keep a mask defined centrally for each platform which
> contains
> >>>>> signals the VM needs for itself ?
> >>>>>
> >>>>>
> >>>>>
> >>>>> +sigset_t os::Aix::sigs = { 0 };
> >>>>>
> >>>>>
> >>>>>
> >>>>> I would not initialize the signal set this way. sigset_t is an opaque
> >>>>> type; the only way to initialize it is with one of sigemptyset() or
> >>>>> sigfillset().
> >>>>>
> >>>>>
> >>>>>
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >>>> NSIG/webrev.01/src/os/aix/vm/os_aix.hpp.udiff.html
> >>>>>
> >>>>>
> >>>>>
> >>>>> + static struct sigaction sigact[NSIG]; // saved preinstalled sigactions
> >>>>>
> >>>>> + static sigset_t sigs; // mask of signals that have
> >>>>>
> >>>>>
> >>>>>
> >>>>> + static int sigflags[NSIG];
> >>>>>
> >>>>>
> >>>>>
> >>>>> I know this is not in the scope of your change, but I would like to see
> >>>>> those removed from os::Aix and put into os_aix.cpp at static
> filescope.
> >>>>> There is no need at all to export those, and you would get rid of the
> >>>>> signal.h dependency you know have when including os_aix.hpp.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >>>> NSIG/webrev.01/src/os/bsd/vm/jsig.c.udiff.html
> >>>>>
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >>>> NSIG/webrev.01/src/os/bsd/vm/os_bsd.cpp.udiff.html
> >>>>>
> >>>>>
> >>>>>
> >>>>> On BSD, we have realtime signals.
> >>>>>
> >>>>>
> >>>>>
> >>>>> http://fxr.watson.org/fxr/source/sys/signal.h
> >>>>>
> >>>>> #define SIGRTMAX 126
> >>>>>
> >>>>> and NSIG does not contain them:
> >>>>>
> >>>>> #define NSIG 32
> >>>>>
> >>>>>
> >>>>>
> >>>>> The max. possible signal number would be 126, which unfortunately
> >> does
> >>>>> not even fit into a 64bit mask.
> >>>>>
> >>>>>
> >>>>>
> >>>>> So the code in jsig.c is broken for the case that someone wants to
> >>>>> register realtime signals, if the VM were to ever use realtime signals
> >>>>> itself, which now is not the case.
> >>>>>
> >>>>>
> >>>>>
> >>>>> The same is true for os_bsd.cpp, where signal chaining will not work if
> >>>>> the application did have handler for real time signals pre-installed
> >>>>> before jvm is loaded.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Solaris:
> >>>>>
> >>>>>
> >>>>>
> >>>>> The only platform where NSIG is missing?
> >>>>>
> >>>>>
> >>>>>
> >>>>> Here, we calculate the max. signal number dynamically in
> os_solaris.cpp,
> >>>>> presumably because SIGRTMAX is not a constant and can be changed
> >> using
> >>>>> system configuration. But then, on Linux we have the same situation
> >>>>> (SIGRTMAX is dynamic) and there we do not go through the trouble
> of
> >>>>> calculating the max. signal number dynamically. Instead we just use
> >>>>> NSIG=64 and rely on the fact that NSIG is larger than the largest
> >>>>> possible dynamic value for SIGRTMAX.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Solaris does not seem to have NSIG defined, but I am sure there is
> also
> >>>>> a max. possible value for SIGRTMAX (the default seems to be 48). So,
> >> one
> >>>>> could probably safely define NSIG for Solaris too, so that we have
> NSIG
> >>>>> defined on all Posix platforms.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Thu, Nov 12, 2015 at 8:24 PM, Lindenmaier, Goetz
> >>>>> <goetz.lindenmaier@sap.com <mailto:goetz.lindenmaier@sap.com>>
> >>>> wrote:
> >>>>>
> >>>>> Hi David, Dmitry,
> >>>>>
> >>>>> I've come up with a new webrev:
> >>>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >> NSIG/webrev.01/
> >>>>>
> >>>>> I hit on some more issues:
> >>>>> - As proposed, I replaced MAXSIGNUM by NSIG
> >>>>> - On AIX, NSIG=255. Therefore storing bits in a word does not
> work.
> >>>>> I'm now using bitset functionality from signal.h as it's done in
> >>>>> other places.
> >>>>> sigset_t is >> NSIG on linux, so it's no good idea to use it there.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Why do we not do this on all platforms, provided sigset_t contains all
> >>>>> signals (incl. realtime signals) ?
> >>>>>
> >>>>>
> >>>>>
> >>>>> - In the os files I found another bit vector that now is too small:
> >>>>> sigs.
> >>>>> I adapted that, too. Removed the dead declaration of this on
> >>>>> solaris.
> >>>>>
> >>>>> Best regards,
> >>>>> Goetz.
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> Kind Regards, Thomas
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> > -----Original Message-----
> >>>>> > From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com
> >>>> <mailto:dmitry.samersoff@oracle.com>]
> >>>>>
> >>>>> > Sent: Donnerstag, 12. November 2015 10:05
> >>>>> > To: Lindenmaier, Goetz; David Holmes; hotspot-runtime-
> >>>>> > dev@openjdk.java.net <mailto:dev@openjdk.java.net>;
> >> serviceability-
> >>>> dev
> >>>>> > Subject: Re: RFR(M): 8141529: Fix handling of _JAVA_SR_SIGNUM
> >>>>> >
> >>>>> > Goetz,
> >>>>> >
> >>>>> > *BSD including OS X also defines NSIG (just checked) and if my
> >>>>> memory is
> >>>>> > not bogus, AIX defines it too.
> >>>>> >
> >>>>> > So you may consider to use NSIG on all platform.
> >>>>> >
> >>>>> > -Dmitry
> >>>>> >
> >>>>> > On 2015-11-12 11:36, Lindenmaier, Goetz wrote:
> >>>>> > > OK I'll change it to NSIG. That's used in other places in
> >>>>> os_linux, too.
> >>>>> > > So it's really more consistent.
> >>>>> > >
> >>>>> > > Best regards,
> >>>>> > > Goetz
> >>>>> > >
> >>>>> > >> -----Original Message-----
> >>>>> > >> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com
> >>>>> <mailto:dmitry.samersoff@oracle.com>]
> >>>>> > >> Sent: Donnerstag, 12. November 2015 09:22
> >>>>> > >> To: David Holmes; Lindenmaier, Goetz; hotspot-runtime-
> >>>>> > >> dev@openjdk.java.net <mailto:dev@openjdk.java.net>;
> >>>>> serviceability-dev
> >>>>> > >> Subject: Re: RFR(M): 8141529: Fix handling of
> _JAVA_SR_SIGNUM
> >>>>> > >>
> >>>>> > >> David,
> >>>>> > >>
> >>>>> > >> I think it's better to use NSIG (without underscore) defined in
> >>>>> signal.h
> >>>>> > >>
> >>>>> > >> -Dmitry
> >>>>> > >>
> >>>>> > >>
> >>>>> > >> On 2015-11-12 10:35, David Holmes wrote:
> >>>>> > >>> Hi Goetz,
> >>>>> > >>>
> >>>>> > >>> Adding in serviceability-dev
> >>>>> > >>>
> >>>>> > >>> On 9/11/2015 6:22 PM, Lindenmaier, Goetz wrote:
> >>>>> > >>>> Hi,
> >>>>> > >>>>
> >>>>> > >>>> The environment variable _JAVA_SR_SIGNUM can be set to
> a
> >>>> signal
> >>>>> > >> number
> >>>>> > >>>> do be used by the JVM's suspend/resume mechanism.
> >>>>> > >>>>
> >>>>> > >>>> If set, a signal handler is installed and the current signal
> >>>>> handler
> >>>>> > >>>> is saved to an array.
> >>>>> > >>>> On linux, this array had size MAXSIGNUM=32, and
> >>>> _JAVA_SR_SIGNUM
> >>>>> > >> was
> >>>>> > >>>> allowed
> >>>>> > >>>> to range up to _NSIG=65. This could cause memory
> corruption.
> >>>>> > >>>>
> >>>>> > >>>> Further, in jsig.c, an unsinged int is used to set a bit for
> >>>>> signals.
> >>>>> > >>>> This also
> >>>>> > >>>> is too small, as only 32 signals can be supported. Further,
> the
> >>>>> > >>>> signals are mapped
> >>>>> > >>>> wrong to these bits. '0' is not a valid signal, but '32'
> >>>>> was. 1<<32
> >>>>> > >>>> happens to map to
> >>>>> > >>>> zero, so the signal could be stored, but this probably was not
> >>>>> > >>>> intended that way.
> >>>>> > >>>>
> >>>>> > >>>> This change increases MAXSIGNUM to 65 on linux, and to 64
> on
> >>>>> aix. It
> >>>>> > >>>> introduces
> >>>>> > >>>> proper checking of the signal read from the env var, and
> issues
> >> a
> >>>>> > >>>> warning if it
> >>>>> > >>>> does not use the signal set. It adapts the data types in jisig.c
> >>>>> > >>>> properly.
> >>>>> > >>>>
> >>>>> > >>>> Please review this change. I please need a sponsor.
> >>>>> > >>>> http://cr.openjdk.java.net/~goetz/webrevs/8141529-
> >>>> NSIG/webrev.00
> >>>>> > >>>
> >>>>> > >>> This all sounds very good to me. (I must find out why Solaris
> >>>>> is not
> >>>>> > >>> involved here :) ).
> >>>>> > >>>
> >>>>> > >>> On Linux you didn't add the bounds check to
> >>>>> os::Linux::set_our_sigflags.
> >>>>> > >>>
> >>>>> > >>> I'm also wondering about documenting where we are
> >> determining
> >>>> the
> >>>>> > >>> maximum from? Is it simply _NSIG on some/all distributions?
> >>>>> And I see
> >>>>> > >>> _NSIG is supposed to be the biggest signal number + one.
> Also
> >>>>> linux
> >>>>> > >>> defines NSIG = _NSIG so which should we be using?
> >>>>> > >>>
> >>>>> > >>> Thanks,
> >>>>> > >>> David
> >>>>> > >>>
> >>>>> > >>>> Best regards,
> >>>>> > >>>> Goetz.
> >>>>> > >>>>
> >>>>> > >>
> >>>>> > >>
> >>>>> > >> --
> >>>>> > >> Dmitry Samersoff
> >>>>> > >> Oracle Java development team, Saint Petersburg, Russia
> >>>>> > >> * I would love to change the world, but they won't give me the
> >>>>> sources.
> >>>>> >
> >>>>> >
> >>>>> > --
> >>>>> > Dmitry Samersoff
> >>>>> > Oracle Java development team, Saint Petersburg, Russia
> >>>>> > * I would love to change the world, but they won't give me the
> >>>>> sources.
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>> --
> >>>> Dmitry Samersoff
> >>>> Oracle Java development team, Saint Petersburg, Russia
> >>>> * I would love to change the world, but they won't give me the
> sources.
> >>
> >>
> >> --
> >> Dmitry Samersoff
> >> Oracle Java development team, Saint Petersburg, Russia
> >> * I would love to change the world, but they won't give me the sources.
>
>
> --
> Dmitry Samersoff
> Oracle Java development team, Saint Petersburg, Russia
> * I would love to change the world, but they won't give me the sources.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic