[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
Ao Qi
aoqi at loongson.cn
Tue Mar 5 10:59:38 UTC 2019
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