RFR: 2178143: VM crashes if the number of bound CPUs changed during runtime

Yumin Qi yumin.qi at oracle.com
Wed Mar 27 12:40:43 PDT 2013


new webrev at same link:

http://cr.openjdk.java.net/~minqi/2178143/

Thanks
Yumin

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