[PATCH] 8170639: [Linux] jsig is limited to a maximum of 64 signals
David Holmes
david.holmes at oracle.com
Tue Mar 5 10:35:31 UTC 2019
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.
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