RFR: 2178143: VM crashes if the number of bound CPUs changed during runtime
Jon Masamitsu
jon.masamitsu at oracle.com
Wed Mar 27 18:17:13 UTC 2013
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