RFR: 2178143: VM crashes if the number of bound CPUs changed during runtime
Jon Masamitsu
jon.masamitsu at oracle.com
Wed Mar 27 20:25:13 UTC 2013
Yumin,
I suggest the addition of PrintGCDetails to address some of David's
concern that basically the warning is not needed. If the user adds
PrintGCDetails to the command line, I think it is an indication that
the user cares about GC so will want to see the warning.
But I'm willing to leave it out if you think every user deserves the
warning.
Jon
On 3/27/2013 12:22 PM, Yumin Qi wrote:
> Jon,
>
> The warning is for a immediate attention, with PrintGCDetail, that
> requires it set on in command line, right?
>
> I also found that with default setting (no GC flavor set), no
> setting for ParallelGCThreads, it will be 0.
>
> so with java -XX:+AssumeMP -version, it will give warning, this is
> not right.
>
> // Set per-collector flags
> if (UseParallelGC || UseParallelOldGC) {
> set_parallel_gc_flags();
> } else if (UseConcMarkSweepGC) { // should be done before ParNew
> check below
> set_cms_and_parnew_gc_flags();
> } else if (UseParNewGC) { // skipped if CMS is set above
> set_parnew_gc_flags();
> } else if (UseG1GC) {
> set_g1_gc_flags();
> }
> check_deprecated_gcs();
> check_deprecated_gc_flags();
>
> With default GC (Btw, what is the default GC collector?),
> ParallelGCThreads = 0.
>
>
>
> So I will change code to:
>
> if (AssumeMP && !UseSerialGC) {
> if (FLAG_IS_DEFAULT(ParallelGCThreads) && ParallelGCThreads == 1) {
> warning("If the number of processors is expected to increase
> from one, then"
> " you should configure the number of parallel GC threads
> appropriately"
> " using -XX:ParallelGCThreads=N");
> }
> }
>
> Will send another webrev after test.
>
> Thanks
> Yumin
>
> On 3/27/2013 11:17 AM, Jon Masamitsu wrote:
>> Yumin,
>>
>> How about adding PrintGCDetails to the guard?
>>
>> if (AssumeMP && !UseSerialGC && PrintGCDetails) {.
>>
>> Jon
>>
>> On 3/27/2013 9:41 AM, Yumin Qi wrote:
>>> so the warning should be
>>>
>>> if (AssumeMP && !UseSerialGC) {
>>> if (FLAG_IS_DEFAULT(ParallelGCThreads) && ParallelGCThreads < 2) {
>>> warning("If the number of processors is expected to increase
>>> from one, then"
>>> " you should configure the number of parallel GC
>>> threads appropriately"
>>> " using -XX:ParallelGCThreads=N");
>>> }
>>> }
>>>
>>> So the warning will only be issued when AssumeMP turned on and the
>>> ncpus =1. If user add cmdline -XX:ParallelGCThreads=1 which is not
>>> the default value 0, so the warning will not be triggered. I will do
>>> a test.
>>>
>>> Is this better?
>>>
>>> Thanks
>>> Yumin
>>>
>>> On 3/26/2013 9:54 PM, David Holmes wrote:
>>>> On 27/03/2013 2:06 PM, Yumin Qi wrote:
>>>>> David,
>>>>>
>>>>> On 3/26/2013 7:49 PM, David Holmes wrote:
>>>>>> Well now you have confused me. I thought checking
>>>>>> FLAG_IS_DEFAULT(ParallelGCThreads) was the right thing to do (the
>>>>>> user
>>>>>> has not attempted manage the number of GC threads even though they
>>>>>> recognize the number of processors may start at 1 and increase
>>>>>> later).
>>>>>> I do not understand the new <2 test - maybe I really only want 1,
>>>>>> if I
>>>>>> asked for that why should I get a warning ???
>>>>>>
>>>>> The previous code is issuing the warning before gc set flags
>>>>> (includes
>>>>> ParallelGCThreads), we don't want with AssumeMP turned on, running
>>>>> on a
>>>>> MP issues the warning since ParallelGCThreads will be not 1 but the
>>>>> number of calculation based on ncpus. So I moved the check code
>>>>> after
>>>>> GC set flags. After GC set flags, ParallelGCThreads is not the
>>>>> default
>>>>> value (0) so I check if it is < 2.
>>>>
>>>> I was overlooking the case where you use +AssumeMP but have
>>>> multiple processors anyway. But if I explicitly set
>>>> ParallelGCThreads=1 then I don't expect to get a warning. Not sure
>>>> we can distinguish these cases though.
>>>>
>>>> Again I leave this to you and Jon to finalize.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> If users really want to run with single CPU with -XX:+AssumeMP,
>>>>> giving a
>>>>> warning is reasonable (this is our intention here) --- there is
>>>>> possibility they will increase number of cpus even they don't,
>>>>> that is OK.
>>>>>
>>>>> Thanks
>>>>> Yumin
>>>>>> But, this is a GC warning (and I think it is misplaced anyway) so if
>>>>>> Jon is happy with this behaviour so be it.
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>> On 27/03/2013 5:44 AM, Yumin Qi wrote:
>>>>>>> Sorry, copied wrong link. It is at same link.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~minqi/2178143/
>>>>>>>
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>
>>>>>>> On 3/26/2013 12:42 PM, Daniel D. Daugherty wrote:
>>>>>>>>
>>>>>>>> On 3/26/13 1:17 PM, Yumin Qi wrote:
>>>>>>>>> Hi, Jon
>>>>>>>>>
>>>>>>>>> I made a change to the warning condition and move it after
>>>>>>>>> GC flags
>>>>>>>>> are set. I think at this time, ParallelGCThreads has a value
>>>>>>>>> != 0.
>>>>>>>>> Please take a look if it is right. The This is the new webrev:
>>>>>>>>>
>>>>>>>>> http://javaweb.us.oracle.com/~yqi/webrev/2178143/
>>>>>>>>
>>>>>>>> This webrev isn't accessible outside Oracle...
>>>>>>>>
>>>>>>>> Dan
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Tested with platform of ncpus > 1 and no ParallelGCThreads
>>>>>>>>> set with
>>>>>>>>> turning on AssumeMP. The warning is gone.
>>>>>>>>> Also tested with ncpus = 1 and the warning is on too.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Yumin
>>>>>>>>>
>>>>>>>>> On 3/26/2013 8:03 AM, Jon Masamitsu wrote:
>>>>>>>>>> If I set AssumeMP on the command line and nothing else
>>>>>>>>>> and am running on a platform where I get ParallelGCThreads > 1,
>>>>>>>>>> am I going to see the warning? If yes, that seems too
>>>>>>>>>> intrusive.
>>>>>>>>>> I can see users hearing about the flag and deciding to include
>>>>>>>>>> it always just to be safe.
>>>>>>>>>>
>>>>>>>>>> Jon
>>>>>>>>>>
>>>>>>>>>> On 3/25/13 11:21 PM, Yumin Qi wrote:
>>>>>>>>>>> You are right, thanks! I misread the INCLUDE.
>>>>>>>>>>> new web in the same link.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Yumin
>>>>>>>>>>>
>>>>>>>>>>> On 3/25/2013 11:00 PM, David Holmes wrote:
>>>>>>>>>>>> On 26/03/2013 3:56 PM, Yumin Qi wrote:
>>>>>>>>>>>>> David,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 3/25/2013 10:41 PM, David Holmes wrote:
>>>>>>>>>>>>>> On 26/03/2013 3:30 PM, Yumin Qi wrote:
>>>>>>>>>>>>>>> David and All,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Changed as suggested by David.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> David, I did not move the warning into
>>>>>>>>>>>>>>> set_parallel_gc_flags()
>>>>>>>>>>>>>>> since
>>>>>>>>>>>>>>> except for serial gc, all other GCs will check this
>>>>>>>>>>>>>>> flag, so I
>>>>>>>>>>>>>>> moved it
>>>>>>>>>>>>>>> into
>>>>>>>>>>>>>>> #if INCLUDE_ALL_GCS
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> before call set GC flags.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I didn't realize all the other GCs would utilize
>>>>>>>>>>>>>> ParallelGCThreads.
>>>>>>>>>>>>>> Can we at least exclude it for serial GC case?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Serial GC is set here:
>>>>>>>>>>>>
>>>>>>>>>>>> INCLUDE_ALL_GCS allows all GCs including Serial GC.
>>>>>>>>>>>> Otherwise we
>>>>>>>>>>>> only have SerialGC. So you have excluded it for the case where
>>>>>>>>>>>> SerialGC is the only GC built into the VM, but not for the
>>>>>>>>>>>> case
>>>>>>>>>>>> where all GCs are available and the user requests UseSerialGC.
>>>>>>>>>>>>
>>>>>>>>>>>> David
>>>>>>>>>>>> -----
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> 3253 #if !INCLUDE_ALL_GCS
>>>>>>>>>>>>> 3254 force_serial_gc();
>>>>>>>>>>>>> 3255 #endif // INCLUDE_ALL_GCS
>>>>>>>>>>>>>
>>>>>>>>>>>>> So after it moved into
>>>>>>>>>>>>> 3283 #if INCLUDE_ALL_GCS
>>>>>>>>>>>>> 3284 if (AssumeMP && FLAG_IS_DEFAULT(ParallelGCThreads)) {
>>>>>>>>>>>>> 3285 warning("If the number of processors is expected to
>>>>>>>>>>>>> increase
>>>>>>>>>>>>> from one, then"
>>>>>>>>>>>>> 3286 " you should configure the number of
>>>>>>>>>>>>> parallel GC
>>>>>>>>>>>>> threads appropriately"
>>>>>>>>>>>>> 3287 " using -XX:ParallelGCThreads=N");
>>>>>>>>>>>>> 3288 }
>>>>>>>>>>>>> 3289 // Set per-collector flags
>>>>>>>>>>>>> 3290 if (UseParallelGC || UseParallelOldGC) {
>>>>>>>>>>>>> 3291 set_parallel_gc_flags();
>>>>>>>>>>>>> 3292 } else if (UseConcMarkSweepGC) { // should be done
>>>>>>>>>>>>> before
>>>>>>>>>>>>> ParNew
>>>>>>>>>>>>> check below
>>>>>>>>>>>>> 3293 set_cms_and_parnew_gc_flags();
>>>>>>>>>>>>> 3294 } else if (UseParNewGC) { // skipped if CMS is set
>>>>>>>>>>>>> above
>>>>>>>>>>>>> 3295 set_parnew_gc_flags();
>>>>>>>>>>>>> 3296 } else if (UseG1GC) {
>>>>>>>>>>>>> 3297 set_g1_gc_flags();
>>>>>>>>>>>>> 3298 }
>>>>>>>>>>>>> 3299 check_deprecated_gcs();
>>>>>>>>>>>>> 3300 #else // INCLUDE_ALL_GCS
>>>>>>>>>>>>> 3301 assert(verify_serial_gc_flags(), "SerialGC unset");
>>>>>>>>>>>>> 3302 #endif // INCLUDE_ALL_GCS
>>>>>>>>>>>>>
>>>>>>>>>>>>> It is excluded for serial GC.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Otherwise looks good.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> new webrev: http://cr.openjdk.java.net/~minqi/2178143/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 3/25/2013 5:44 PM, David Holmes wrote:
>>>>>>>>>>>>>>>> Hi Yumin,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I have a few suggested changes.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> First in os.hpp I suggest
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> return _processor_count > 1 || AssumeMP ;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> That way we don't bypass the assertion and don't encounter
>>>>>>>>>>>>>>>> unnecessary
>>>>>>>>>>>>>>>> overhead on the common path.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In arguments.cpp:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> + if (AssumeMP && FLAG_IS_DEFAULT(ParallelGCThreads)) {
>>>>>>>>>>>>>>>> + warning("With AssumeMP, recommend run with
>>>>>>>>>>>>>>>> -XX:ParallelGCThreads=<num_of_gc_threads>, where"
>>>>>>>>>>>>>>>> + " num_of_gc_threads can be set to number of
>>>>>>>>>>>>>>>> cores");
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I'm not convinced issuing a warning is necessary or
>>>>>>>>>>>>>>>> desirable
>>>>>>>>>>>>>>>> here.
>>>>>>>>>>>>>>>> With a uniprocessor startup will we even default to using
>>>>>>>>>>>>>>>> parallel GC?
>>>>>>>>>>>>>>>> The above should only be issued if using parallel GC - so
>>>>>>>>>>>>>>>> better to
>>>>>>>>>>>>>>>> move it into set_parallel_gc_flags().
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In any case it isn't clear what the user should supply
>>>>>>>>>>>>>>>> here.
>>>>>>>>>>>>>>>> If they
>>>>>>>>>>>>>>>> use the maximum expected number of cores initially then
>>>>>>>>>>>>>>>> while
>>>>>>>>>>>>>>>> they
>>>>>>>>>>>>>>>> only have 1 core their VM may not even function
>>>>>>>>>>>>>>>> adequately due
>>>>>>>>>>>>>>>> to GC
>>>>>>>>>>>>>>>> overhead. I think all you can say here is something like:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> warning("If the number of processors is expected to
>>>>>>>>>>>>>>>> increase
>>>>>>>>>>>>>>>> from one,
>>>>>>>>>>>>>>>> then you should configure the number of parallel GC
>>>>>>>>>>>>>>>> threads
>>>>>>>>>>>>>>>> appropriately using -XX:ParallelGCThreads=N");
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In globals.hpp:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> + product(bool, AssumeMP, false, \
>>>>>>>>>>>>>>>> + "Assume run on multiple processors always") \
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Was there any discussion on making this default to true
>>>>>>>>>>>>>>>> instead?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Suggested re-wording:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> "Instruct the VM to assume multiple processors are
>>>>>>>>>>>>>>>> available"
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 26/03/2013 9:15 AM, Yumin Qi wrote:
>>>>>>>>>>>>>>>>> It should be "AssumeMP".
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> /Yumin
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 3/25/2013 3:32 PM, Yumin Qi wrote:
>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> New webrev to use "-XX:+AssumMP" flag. Also add
>>>>>>>>>>>>>>>>>> warning to
>>>>>>>>>>>>>>>>>> recommend
>>>>>>>>>>>>>>>>>> use this flag with "-XX:ParallelGCThreads" to remind
>>>>>>>>>>>>>>>>>> user to
>>>>>>>>>>>>>>>>>> avoid
>>>>>>>>>>>>>>>>>> running with only one GC thread. This is verified by
>>>>>>>>>>>>>>>>>> Oleksandr with
>>>>>>>>>>>>>>>>>> the test case running on Linux which is not Zone
>>>>>>>>>>>>>>>>>> configured.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Same link.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 3/20/2013 2:27 PM, Yumin Qi wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 2178143: VM crashes if the number of bound CPUs
>>>>>>>>>>>>>>>>>>> changed
>>>>>>>>>>>>>>>>>>> during
>>>>>>>>>>>>>>>>>>> runtime.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Situation: Customer first configure only one CPU
>>>>>>>>>>>>>>>>>>> online and
>>>>>>>>>>>>>>>>>>> turn
>>>>>>>>>>>>>>>>>>> others offline to run java application, after java
>>>>>>>>>>>>>>>>>>> program
>>>>>>>>>>>>>>>>>>> started,
>>>>>>>>>>>>>>>>>>> bring more CPUs back online. Since VM started on a
>>>>>>>>>>>>>>>>>>> single
>>>>>>>>>>>>>>>>>>> CPU,
>>>>>>>>>>>>>>>>>>> os::is_MP() will return false, but after more CPUs
>>>>>>>>>>>>>>>>>>> available, OS
>>>>>>>>>>>>>>>>>>> will
>>>>>>>>>>>>>>>>>>> schedule the app run on multiple CPUs, this caused
>>>>>>>>>>>>>>>>>>> SEGV in
>>>>>>>>>>>>>>>>>>> various
>>>>>>>>>>>>>>>>>>> places where data consistency was broken. The
>>>>>>>>>>>>>>>>>>> solution is
>>>>>>>>>>>>>>>>>>> supply a
>>>>>>>>>>>>>>>>>>> flag to assume it is running on MP, so lock is
>>>>>>>>>>>>>>>>>>> forced to be
>>>>>>>>>>>>>>>>>>> called.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~minqi/2178143/
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>> Yumin
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>
More information about the hotspot-gc-dev
mailing list