[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
Ao Qi
aoqi at loongson.cn
Mon Mar 11 08:09:08 UTC 2019
Hi David,
On Mon, Mar 11, 2019 at 3:59 PM David Holmes <david.holmes at oracle.com> wrote:
>
> On 11/03/2019 4:10 pm, David Holmes wrote:
> > Hi,
> >
> > Sorry was on a few days vacation.
> >
> > I will run this through our test system and report back.
Thank you very much!
>
> All passed.
Happy to hear that. David, are you okay with this patch? Do I need
another review?
Ao Qi
>
> David
> -----
>
> > David
> >
> > On 5/03/2019 8:59 pm, Ao Qi wrote:
> >> On Tue, Mar 5, 2019 at 6:35 PM David Holmes <david.holmes at oracle.com>
> >> wrote:
> >>>
> >>> On 5/03/2019 8:25 pm, Ao Qi wrote:
> >>>> Hi David,
> >>>>
> >>>> Webrev: http://cr.openjdk.java.net/~aoqi/8170639/webrev.03
> >>>>
> >>>> What about this version? Am I understanding correctly? I think this
> >>>> version is indeed much better.
> >>>
> >>> Yes that's what I meant.
> >>>
> >>>> I also fixed a problem in my previous
> >>>> version:( The code was only put into #ifdef SUPPORTS_CLOCK_MONOTONIC
> >>>> block.
> >>>
> >>> Oops missed that. Surprised the declarations in the .hpp file don't also
> >>> need to be in #ifndef SOLARIS.
> >>>
> >>>> Done tests:
> >>>> build: linux-x86_64-{server, minimal, zero}-{fastdebug, release},
> >>>> solaris-x86_64-server-release
> >>>> jtreg: linux-x86_64-server-release: hotspot:tier1
> >>>>
> >>>> I am doing other tests. Seems good so far.
> >>>
> >>> Please run through the submit repo if you can.
> >>>
> >>
> >> I am afraid I cannot, because I am not a Committer. Maybe I need some
> >> Committer to help me to run the tests. Could someone help?
> >>
> >> Thanks,
> >> Ao Qi
> >>
> >>> Thanks,
> >>> David
> >>>
> >>>> Thanks,
> >>>> Ao Qi
> >>>>
> >>>> On Tue, Mar 5, 2019 at 10:45 AM David Holmes
> >>>> <david.holmes at oracle.com> wrote:
> >>>>>
> >>>>> On 5/03/2019 12:22 pm, Ao Qi wrote:
> >>>>>>
> >>>>>>
> >>>>>> David Holmes <david.holmes at oracle.com
> >>>>>> <mailto:david.holmes at oracle.com>>
> >>>>>> 于2019年3月1日 周五下午12:18写道：
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 28/02/2019 9:01 pm, Ao Qi wrote:
> >>>>>> > Hi,
> >>>>>> >
> >>>>>> > This version is more consolidated than my first version
> >>>>>> in this
> >>>>>> > thread:
> >>>>>> http://cr.openjdk.java.net/~aoqi/8170639/webrev.01/. I filed
> >>>>>> > my first version here:
> >>>>>> > http://cr.openjdk.java.net/~aoqi/8170639/webrev.00/
> >>>>>> >
> >>>>>> > I fixed the linux and bsd using sigset_t, so jsig is not
> >>>>>> limited to a
> >>>>>> > maximum of 64 signals for linux and bsd. The logic of aix
> >>>>>> and solaris
> >>>>>> > is not changed.
> >>>>>>
> >>>>>> Thanks for doing this. Overall this is a much better approach.
> >>>>>>
> >>>>>> I have two requests.
> >>>>>>
> >>>>>> There are too many ifdefs for solaris in the os_posix.cpp
> >>>>>> code. I
> >>>>>> hadn't
> >>>>>> realized the difference between Solaris and the other *NIX
> >>>>>> platforms in
> >>>>>> this area (allowing for real-time signal chaining). Please
> >>>>>> restore the
> >>>>>> Solaris code and put one ifndef solaris around the new code is
> >>>>>> os_posix.[ch]pp.
> >>>>>>
> >>>>>>
> >>>>>> Hi David,
> >>>>>>
> >>>>>> Thanks for your review and advice.
> >>>>>>
> >>>>>> I think the best way is to eliminate ifdefs for solaris, but as
> >>>>>> you said
> >>>>>> in a previous email [1]: "SIGRTMAX is a macro not a constant so you
> >>>>>> can't declare a static array using it. You would need to malloc the
> >>>>>> array as done on Solaris." So I have to use ifdefs for solaris, I did
> >>>>>> not find a way to eliminate it. I have used one ifndef solaris as you
> >>>>>> request.
> >>>>>
> >>>>> What I was suggesting was to leave os_solaris.cpp alone completely for
> >>>>> this change, and have the new os_posix.* functions defined in an
> >>>>> "#ifndef SOLARIS" block.
> >>>>>
> >>>>> Thanks,
> >>>>> David
> >>>>>
> >>>>> PS. I will be away for a few days after today.
> >>>>>
> >>>>>> There's no need for a separate os::posix::init_sigs() API,
> >>>>>> just do the
> >>>>>> signal set initialization in os::posix::init_2();
> >>>>>>
> >>>>>>
> >>>>>> I have done it.
> >>>>>>
> >>>>>> Webrev: http://cr.openjdk.java.net/~aoqi/8170639/webrev.02/
> >>>>>>
> >>>>>> Tested:
> >>>>>> build tests
> >>>>>> linux-x86_64-minimal-fastdebug
> >>>>>> linux-x86_64-zero-fastdebug
> >>>>>> linux-x86_64-server-fastdebug
> >>>>>> linux-x86_64-minimal-release
> >>>>>> linux-x86_64-zero-release
> >>>>>> linux-x86_64-server-release
> >>>>>> solaris-x86_64-server-release
> >>>>>> solaris-x86_64-server-fastdebug
> >>>>>> linux-mips64el-zero-release
> >>>>>> linux-mips64el-zero-fastdebug
> >>>>>>
> >>>>>> jtreg tests
> >>>>>> linux-x86_64-server-release: tier1,
> >>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:-UseSignalChaining),
> >>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:+AllowUserSignalHandlers)
> >>>>>> solaris-x86_64-server-release: hotspot-tier1,
> >>>>>> hotspot-tier1(JAVA_OPTIONS=-XX:-UseSignalChaining)
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Ao Qi
> >>>>>>
> >>>>>> [1]
> >>>>>> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-March/027219.html
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> David
> >>>>>> -----
> >>>>>>
> >>>>>> > The tests I did:
> >>>>>> >
> >>>>>> > build tests：
> >>>>>> > linux-x86_64-server-release
> >>>>>> > linux-x86_64-server-fastdebug
> >>>>>> > linux-x86_64-zero-release
> >>>>>> > linux-x86_64-zero-fastdebug
> >>>>>> > linux-x86_64-minimal-release
> >>>>>> > linux-x86_64-minimal-fastdebug
> >>>>>> > solaris-x86_64-server-release
> >>>>>> > solaris-x86_64-server-fastdebug
> >>>>>> > linux-mips64el-zero-release
> >>>>>> > linux-mips64el-zero-fastdebug
> >>>>>> >
> >>>>>> > jtreg tests：
> >>>>>> > linux-x86_64-server-release: tier1, tier2, tier3
> >>>>>> > linux-x86_64-server-fastdebug: tier1
> >>>>>> > solaris-x86_64-server-release: hotspot-tier1
> >>>>>> >
> >>>>>> > No regression was found. Could you please review this
> >>>>>> version?
> >>>>>> >
> >>>>>> > On Fri, Jan 25, 2019 at 10:56 PM Magnus Ihse Bursie
> >>>>>> > <magnus.ihse.bursie at oracle.com
> >>>>>> <mailto:magnus.ihse.bursie at oracle.com>> wrote:
> >>>>>> >>
> >>>>>> >> On 2018-12-18 01:33, David Holmes wrote:
> >>>>>> >>> On 18/12/2018 9:57 am, Ao Qi wrote:
> >>>>>> >>>> Hi,
> >>>>>> >>>>
> >>>>>> >>>> On Tue, Dec 18, 2018 at 4:32 AM David Holmes
> >>>>>> >>>> <david.holmes at oracle.com
> >>>>>> <mailto:david.holmes at oracle.com>> wrote:
> >>>>>> >>>>>
> >>>>>> >>>>> On 17/12/2018 9:48 pm, Ao Qi wrote:
> >>>>>> >>>>>> Hi David Holmes,
> >>>>>> >>>>>>
> >>>>>> >>>>>> Thanks for your reply. It makes sense. Is it
> >>>>>> possible to create
> >>>>>> >>>>>> another bug ID to fix the problem?
> >>>>>> >>>>>
> >>>>>> >>>>> I'd prefer to see this fixed properly than point
> >>>>>> fixed on
> >>>>>> linux for an
> >>>>>> >>>>> unsupported platform.
> >>>>>> >>>>
> >>>>>> >>>> I thought the problem is "[Linux] jsig is limited to a
> >>>>>> maximum
> >>>>>> of 64
> >>>>>> >>>> signals" and the patch was intended to fixed this
> >>>>>> problem. It just
> >>>>>> >>>> didn't use "A better solution", but I think it is
> >>>>>> better than the
> >>>>>> >>>> current situation.
> >>>>>> >>>
> >>>>>> >>> Better for you but I would prefer to see an overall better
> >>>>>> solution.
> >>>>>> >>> It really shouldn't be very difficult. Compare the AIX
> >>>>>> version with
> >>>>>> >>> your improved Linux version, reconcile any differences,
> >>>>>> and
> >>>>>> make it a
> >>>>>> >>> shared posix version, then confirm it works on OS X and
> >>>>>> Solaris.
> >>>>>> >>>
> >>>>>> >>>> Is zero/mips (or all platforms having more than 64
> >>>>>> signals) a
> >>>>>> >>>> unsupported platform?
> >>>>>> >>>
> >>>>>> >>> Yes. We make allowances for Zero as it can support a
> >>>>>> vast range of
> >>>>>> >>> platforms for which no native OpenJDK port exists (in
> >>>>>> the case
> >>>>>> of MIPS
> >>>>>> >>> that port seems inactive), but in general we don't go
> >>>>>> out of
> >>>>>> our way
> >>>>>> >>> to make changes in the main code that only benefit such
> >>>>>> platforms.
> >>>>>> >>>
> >>>>>> >>> I'm subtly trying to coerce you into making the change
> >>>>>> I'd like
> >>>>>> to see
> >>>>>> >>> here. ;-) I can assist with build/test on other POSIX
> >>>>>> platforms.
> >>>>>> >> If you are going down that route, I recommend looking at
> >>>>>> >> src/java.base/unix/native/libjsig/jsig.c for
> >>>>>> inspiration. It was
> >>>>>> >> recently unified across all unix platforms we currently
> >>>>>> support,
> >>>>>> so any
> >>>>>> >> special treatment there for a specific platform (I think
> >>>>>> solaris, macosx
> >>>>>> >> and aix have some special stuff going on), is likely to
> >>>>>> apply to the
> >>>>>> >> signal handling inside the VM as well. On the other
> >>>>>> hand, general
> >>>>>> >> solutions that worked in jsig.c is likely to work as
> >>>>>> well in the VM.
> >>>>>> >>
> >>>>>> >
> >>>>>> > Hi Magnus,
> >>>>>> >
> >>>>>> > Thanks for you information and advise. That is very helpful.
> >>>>>> > Difference between
> >>>>>> hotspot/os/${os_name}/os_${os_name}.cpp is much
> >>>>>> > more than the difference between
> >>>>>> ${os_name}/native/libjsig/jsig.c, so
> >>>>>> > it is difficult to unify all the jsig related codes into
> >>>>>> os_posix.hpp
> >>>>>> > and os_posix.cpp. Maybe I would try to do it in another
> >>>>>> issue.
> >>>>>> >
> >>>>>> > Cheers,
> >>>>>> > Ao Qi
> >>>>>> >
> >>>>>> >> /Magnus
> >>>>>> >>>
> >>>>>> >>> Cheers,
> >>>>>> >>> David
> >>>>>> >>>
> >>>>>> >>>> Cheers,
> >>>>>> >>>> Ao Qi
> >>>>>> >>>>>
> >>>>>> >>>>> Thanks,
> >>>>>> >>>>> David
> >>>>>> >>>>>
> >>>>>> >>>>>> Cheers,
> >>>>>> >>>>>> Ao Qi
> >>>>>> >>>>>>
> >>>>>> >>>>>> On Mon, Dec 17, 2018 at 7:30 PM David Holmes
> >>>>>> >>>>>> <david.holmes at oracle.com
> >>>>>> <mailto:david.holmes at oracle.com>>
> >>>>>> wrote:
> >>>>>> >>>>>>>
> >>>>>> >>>>>>> Thanks for reminding me about the rest of the
> >>>>>> discussion. The
> >>>>>> >>>>>>> point of
> >>>>>> >>>>>>> the RFE I filed was to examine if we could get a
> >>>>>> consolidated POSIX
> >>>>>> >>>>>>> version using sigset_t. I'd rather see that than a
> >>>>>> point
> >>>>>> fix for
> >>>>>> >>>>>>> Linux.
> >>>>>> >>>>>>>
> >>>>>> >>>>>>> Thanks,
> >>>>>> >>>>>>> David
> >>>>>> >>>>>>>
> >>>>>> >>>>>>> On 17/12/2018 6:50 pm, Ao Qi wrote:
> >>>>>> >>>>>>>> Hi David Holmes,
> >>>>>> >>>>>>>>
> >>>>>> >>>>>>>> I am not sure what is the specific issue of "NSIG
> >>>>>> not being
> >>>>>> >>>>>>>> defined".
> >>>>>> >>>>>>>> Do you mean the issue mentioned in "but I found
> >>>>>> that NSIG was
> >>>>>> >>>>>>>> missing
> >>>>>> >>>>>>>> from signal.h on some architectures, mips being among
> >>>>>> them"[1]? If
> >>>>>> >>>>>>>> yes, James Cowgill answered this in [2]. I check
> >>>>>> it on
> >>>>>> MIPS, NSIG is
> >>>>>> >>>>>>>> defined in signal.h and it is 128.
> >>>>>> >>>>>>>>
> >>>>>> >>>>>>>> Cheers,
> >>>>>> >>>>>>>> Ao Qi
> >>>>>> >>>>>>>>
> >>>>>> >>>>>>>> [1]
> >>>>>> >>>>>>>>
> >>>>>>
> >>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-November/025401.html
> >>>>>>
> >>>>>> >>>>>>>> [2]
> >>>>>> >>>>>>>>
> >>>>>>
> >>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-December/025416.html
> >>>>>>
> >>>>>> >>>>>>>>
> >>>>>> >>>>>>>> On Mon, Dec 17, 2018 at 8:02 AM David Holmes
> >>>>>> >>>>>>>> <david.holmes at oracle.com
> >>>>>> <mailto:david.holmes at oracle.com>>
> >>>>>> wrote:
> >>>>>> >>>>>>>>>
> >>>>>> >>>>>>>>> Hi,
> >>>>>> >>>>>>>>>
> >>>>>> >>>>>>>>> Was the issue of NSIG not being defined resolved?
> >>>>>> >>>>>>>>>
> >>>>>> >>>>>>>>> Thanks,
> >>>>>> >>>>>>>>> David
> >>>>>> >>>>>>>>>
> >>>>>> >>>>>>>>> On 14/12/2018 10:23 pm, Ao Qi wrote:
> >>>>>> >>>>>>>>>> Hi all,
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> Zero does not build on Linux MIPS (actually all
> >>>>>> Linux of
> >>>>>> NSIG >
> >>>>>> >>>>>>>>>> 64)
> >>>>>> >>>>>>>>>> since OpenJDK 9, because NISG is 128 on MIPS and
> >>>>>> can not be
> >>>>>> >>>>>>>>>> encoded in
> >>>>>> >>>>>>>>>> uint64_t sigs. James Cowgill from Debian tried
> >>>>>> to push a
> >>>>>> patch
> >>>>>> >>>>>>>>>> fixing
> >>>>>> >>>>>>>>>> this[1] but it seems he failed. I would like to
> >>>>>> try again to
> >>>>>> >>>>>>>>>> request
> >>>>>> >>>>>>>>>> fixing this in
> >>>>>> http://hg.openjdk.java.net/jdk/jdk. This was
> >>>>>> >>>>>>>>>> fixed in
> >>>>>> >>>>>>>>>> AIX implementation.
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> After applying this patch, zero can be built on
> >>>>>> MIPS (Debian
> >>>>>> >>>>>>>>>> testing).
> >>>>>> >>>>>>>>>> I've run the jtreg tests [2] on x86 Linux.
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> [1]
> >>>>>> >>>>>>>>>>
> >>>>>>
> >>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-November/025399.html
> >>>>>>
> >>>>>> >>>>>>>>>> [2]
> >>>>>> >>>>>>>>>>
> >>>>>>
> >>>>>> https://download.java.net/openjdk/testresults/12/docs/howtoruntests.html
> >>>>>>
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> Cheers,
> >>>>>> >>>>>>>>>> Ao Qi
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> $ hg export -r 52869
> >>>>>> >>>>>>>>>> # HG changeset patch
> >>>>>> >>>>>>>>>> # User aoqi
> >>>>>> >>>>>>>>>> # Date 1544089853 0
> >>>>>> >>>>>>>>>> # Thu Dec 06 09:50:53 2018 +0000
> >>>>>> >>>>>>>>>> # Node ID 3eea22b79dc3a4bf26616bde1acb587e3f56e6fe
> >>>>>> >>>>>>>>>> # Parent b4982a22926b4ddf1a7b1f770e4d42ce8c1dd575
> >>>>>> >>>>>>>>>> 8170639: [Linux] jsig is limited to a maximum of
> >>>>>> 64 signals
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> diff -r b4982a22926b -r 3eea22b79dc3
> >>>>>> >>>>>>>>>> src/hotspot/os/linux/os_linux.cpp
> >>>>>> >>>>>>>>>> --- a/src/hotspot/os/linux/os_linux.cpp Thu Dec
> >>>>>> 06 11:54:39
> >>>>>> >>>>>>>>>> 2018 +0530
> >>>>>> >>>>>>>>>> +++ b/src/hotspot/os/linux/os_linux.cpp Thu Dec
> >>>>>> 06 09:50:53
> >>>>>> >>>>>>>>>> 2018 +0000
> >>>>>> >>>>>>>>>> @@ -4434,10 +4434,7 @@
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> // For signal-chaining
> >>>>>> >>>>>>>>>> struct sigaction sigact[NSIG];
> >>>>>> >>>>>>>>>> -uint64_t sigs = 0;
> >>>>>> >>>>>>>>>> -#if (64 < NSIG-1)
> >>>>>> >>>>>>>>>> -#error "Not all signals can be encoded in sigs.
> >>>>>> Adapt
> >>>>>> its type!"
> >>>>>> >>>>>>>>>> -#endif
> >>>>>> >>>>>>>>>> +sigset_t sigs;
> >>>>>> >>>>>>>>>> bool os::Linux::libjsig_is_loaded = false;
> >>>>>> >>>>>>>>>> typedef struct sigaction
> >>>>>> *(*get_signal_t)(int);
> >>>>>> >>>>>>>>>> get_signal_t os::Linux::get_signal_action
> >>>>>> = NULL;
> >>>>>> >>>>>>>>>> @@ -4516,7 +4513,7 @@
> >>>>>> >>>>>>>>>> }
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> struct sigaction*
> >>>>>> os::Linux::get_preinstalled_handler(int
> >>>>>> >>>>>>>>>> sig) {
> >>>>>> >>>>>>>>>> - if ((((uint64_t)1 << (sig-1)) & sigs) != 0) {
> >>>>>> >>>>>>>>>> + if (sigismember(&sigs, sig)) {
> >>>>>> >>>>>>>>>> return &sigact[sig];
> >>>>>> >>>>>>>>>> }
> >>>>>> >>>>>>>>>> return NULL;
> >>>>>> >>>>>>>>>> @@ -4525,7 +4522,7 @@
> >>>>>> >>>>>>>>>> void
> >>>>>> os::Linux::save_preinstalled_handler(int sig,
> >>>>>> struct
> >>>>>> >>>>>>>>>> sigaction& oldAct) {
> >>>>>> >>>>>>>>>> assert(sig > 0 && sig < NSIG, "vm signal
> >>>>>> out of
> >>>>>> expected
> >>>>>> >>>>>>>>>> range");
> >>>>>> >>>>>>>>>> sigact[sig] = oldAct;
> >>>>>> >>>>>>>>>> - sigs |= (uint64_t)1 << (sig-1);
> >>>>>> >>>>>>>>>> + sigaddset(&sigs, sig);
> >>>>>> >>>>>>>>>> }
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> // for diagnostic
> >>>>>> >>>>>>>>>> @@ -4616,6 +4613,7 @@
> >>>>>> >>>>>>>>>> (*begin_signal_setting)();
> >>>>>> >>>>>>>>>> }
> >>>>>> >>>>>>>>>>
> >>>>>> >>>>>>>>>> + ::sigemptyset(&sigs);
> >>>>>> >>>>>>>>>> set_signal_handler(SIGSEGV, true);
> >>>>>> >>>>>>>>>> set_signal_handler(SIGPIPE, true);
> >>>>>> >>>>>>>>>> set_signal_handler(SIGBUS, true);
> >>>>>> >>>>>>>>>>
> >>>>>> >>
> >>>>>>
More information about the hotspot-runtime-dev
mailing list