[16] RFR(S) 8249672: Include microcode revision in features_string on x86

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon Jul 20 22:37:11 UTC 2020


Looks good.

Passed my tier1 testing.

Thanks,
Vladimir

On 7/20/20 10:12 AM, Ivanov, Vladimir A wrote:
> HI,
> The updated patch available as http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.03/
> It use the ‘fgets’ instead of ‘getline’ to use local memory.
> The tier1 tests passed on the release and fastdebug builds on Linux and fastdebug builds on MacOS systems.
> Testing results same for patched and non-patched builds.
> 
> Thanks, Vladmir
> 
> From: Thomas Stüfe <thomas.stuefe at gmail.com>
> Sent: Friday, July 17, 2020 10:25 PM
> To: Ivanov, Vladimir A <vladimir.a.ivanov at intel.com>
> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>; hotspot-compiler-dev at openjdk.java.net
> Subject: Re: [16] RFR(S) 8249672: Include microcode revision in features_string on x86
> 
> Oh, sorry, you are right :(
> 
> I was under the assumption you wanted to call os::cpu_microcode_revision() directly from within VMError::report(). During initialization using c-heap like this should not be a problem and you can forget about 9/10ths of what I wrote, sorry.
> 
> In that case your original variant is fine, my only suggestion would be to clearly mark the free as ::free() with a comment to prevent someone from correcting it to os::free.
> 
> Thank you,
> 
> Thomas
> 
> 
> 
> On Sat, Jul 18, 2020 at 7:08 AM Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
> Hi,
> seems, this info created during initialization phase. Is it correct? Collect or parse common info at the crash point usually not a good idea. During initialization usage of the c-heap not a problem.
> The “::free” work OK here. At least tier1 test produce same results for patched and non-patched builds. But these tests not generates real case for hs_err files.
> It looks like 2k byte array enough for the one record for CPU from cpuinfo file. Will update code to use local buffer.
> 
> Thanks, Vladimir
> 
> From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
> Sent: Friday, July 17, 2020 9:42 PM
> To: Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>
> Cc: Vladimir Kozlov <vladimir.kozlov at oracle.com<mailto:vladimir.kozlov at oracle.com>>; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>; hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: [16] RFR(S) 8249672: Include microcode revision in features_string on x86
> 
> Hi,
> 
> yes, you must use the raw free here (for the same reason we cannot pass in an os::malloc() allocated buffer to getline, since if it were to resize it would use raw ::realloc() internally and crash the same way).
> 
> But as I wrote in my first mail to the original thread, I would not use c-heap memory at all, since this function is used during crash reporting in the signal handler and the c-heap may be corrupted.
> 
> It the max line length of /proc/cpu can be reliably predicted (so that getline wont realloc()) I would pass a stack allocated buffer into getline. If not, I would not use getline() at all but rewrite this, probably using fgets().
> 
> Cheers, Thomas
> 
> 
> 
> 
> On Sat, Jul 18, 2020 at 1:24 AM Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
> Thanks, I expected the C's functions here. Let's wait a little bit for Runtime team and update work with buffer.
> 
>   Thanks, Vladimir
> 
> -----Original Message-----
> From: Vladimir Kozlov <vladimir.kozlov at oracle.com<mailto:vladimir.kozlov at oracle.com>>
> Sent: Friday, July 17, 2020 4:17 PM
> To: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>; Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>
> Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>; hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: [16] RFR(S) 8249672: Include microcode revision in features_string on x86
> 
> I think the issue is 'line' buffer is allocated by libc getline() and os:free() which is HotSpot function [1] does not know about it. You need C's ::free() or use HS's os::malloc() to allocate 'line' buffer.
> 
> Someone from Runtime may suggest what is the best for this case.
> 
> Thanks,
> Vladimir K
> 
> [1] http://hg.openjdk.java.net/jdk/jdk/file/14f465f62984/src/hotspot/share/runtime/os.cpp#l792
> 
> On 7/17/20 4:03 PM, Vladimir Kozlov wrote:
>> I updated subject to our formal review request format (JDK version, RFE's id and subject).
>>
>> I moved RFE to runtime group as Thomas said:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8249672
>>
>> Submitted tier1 testing to build on all our supported platforms. And debug builds on linux failed:
>>
>> #  SIGSEGV (0xb) at pc=0x0000146fc6af4b0b, pid=9715, tid=9718 # V
>> [libjvm.so+0xc12b0b]  GuardedMemory::print_on(outputStream*)
>> const+0xeb
>>
>> V  [libjvm.so+0xc12b0b]  GuardedMemory::print_on(outputStream*)
>> const+0xeb V  [libjvm.so+0x13c898a]  verify_memory(void*)+0x26a V
>> [libjvm.so+0x13cd30b]  os::free(void*)+0x5b V  [libjvm.so+0x13e5598]
>> os::cpu_microcode_revision()+0xc8 V  [libjvm.so+0x17d314c]
>> VM_Version::get_processor_features()+0x76c
>> V  [libjvm.so+0x17d6ead]  VM_Version::initialize()+0x10d V
>> [libjvm.so+0x17ce6c6]  VM_Version_init()+0x26 V  [libjvm.so+0xcb2895]
>> init_globals()+0x55 V  [libjvm.so+0x16dde63]
>> Threads::create_vm(JavaVMInitArgs*, bool*)+0x2d3
>>
>>
>> Regards,
>> Vladimir K
>>
>> On 7/17/20 3:02 PM, Thomas Stüfe wrote:
>>> Hi Vladimir,
>>>
>>> On Fri, Jul 17, 2020 at 11:57 PM Ivanov, Vladimir A <
>>> vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
>>>
>>>>>    +#if defined(IA32) || defined(AMD64)
>>>>>
>>>>> Is that not synonymous with x86?
>>>>
>>>> This patter was copied from the method “print_model_name_and_flags”
>>>> (file os/linux/os_linux.cpp).
>>>>
>>>> This method also read the “/proc/cpuinfo” file and I reuse it as
>>>> ‘template’ for the new method.
>>>>
>>>> It is better to use one pattern to work with exactly same file but
>>>> in general you are right.
>>>>
>>>> The X86 is defined in the file ./share/utilities/macros.hpp as:
>>>>
>>>> #if defined(IA32) || defined(AMD64)
>>>>
>>>> #define X86
>>>>
>>>> #define X86_ONLY(code) code
>>>>
>>>> #define NOT_X86(code)
>>>>
>>>>
>>>>
>>>> The question here: could I delete this “ifdefs” while this method
>>>> should work on x86 only?
>>>>
>>>>
>>>>
>>>
>>> os_linux_x86.cpp is compiled for x86 platforms only, whereas
>>> os_linux.cpp is shared among all architectures.
>>>
>>> So, in the former you do not need to exclude non-x86 architectures.
>>>
>>> Cheers, Thomas
>>>
>>>
>>>> Thanks, Vladimir
>>>>
>>>>
>>>>
>>>> *From:* Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
>>>> *Sent:* Friday, July 17, 2020 2:26 PM
>>>> *To:* Ivanov, Vladimir A <vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>>; Hotspot dev
>>>> runtime <hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>>
>>>> *Cc:* hotspot-compiler-dev at openjdk.java.net<mailto:hotspot-compiler-dev at openjdk.java.net>
>>>> *Subject:* Re: add microcode version to the hs_err files
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Jul 17, 2020 at 11:19 PM Thomas Stüfe
>>>> <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
>>>> wrote:
>>>>
>>>> Hi Vladimir,
>>>>
>>>>
>>>>
>>>> I think this would be more suited to hotspot-runtime.
>>>>
>>>>
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.00/
>>>> src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp.udiff.html
>>>>
>>>>
>>>>
>>>> +#if defined(IA32) || defined(AMD64)
>>>>
>>>> Is that not synonymous with x86?
>>>>
>>>>
>>>>
>>>> +    while ((read = getline(&line, &len, fp)) != -1) {
>>>> +      if (len > 10 && strstr(line, "microcode") != NULL) {
>>>> +        char* rev = strchr(line, ':');
>>>> +        if (rev != NULL) sscanf(rev + 1, "%x", &result);
>>>> +        break;
>>>> +      }
>>>> +    }
>>>> +    free(line);
>>>>
>>>>
>>>>
>>>> Not sure this works as intended. At the first call to getline() it
>>>> will allocate a line buffer for you and return it. That buffer will
>>>> be as large as the first line you happen to read. You then pass that
>>>> same buffer into getline to fetch the next lines, but what if those
>>>> are longer than the first?
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Forget that point, getline calls realloc() on the line buffer to
>>>> resize it, so this should be okay.
>>>>
>>>>
>>>>
>>>> Thanks, Thomas
>>>>
>>>>
>>>>
>>>> But anyway it would be better to pass a simple caller provided
>>>> buffer in - stack allocated. Since this function is called at crash
>>>> time and the C heap could be corrupted.
>>>>
>>>>
>>>>
>>>> Cheers, Thomas
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Jul 17, 2020 at 10:22 PM Ivanov, Vladimir A <
>>>> vladimir.a.ivanov at intel.com<mailto:vladimir.a.ivanov at intel.com>> wrote:
>>>>
>>>> Hello,
>>>>
>>>> could you please review the patch
>>>> http://cr.openjdk.java.net/~sviswanathan/Vladimir/8249672/webrev.00/
>>>>
>>>> This patch add the microcode version for different OSes that may be
>>>> useful in the issue resolution process.
>>>>
>>>>
>>>>
>>>> The reported microcode version for different OSes loos as:
>>>>
>>>>
>>>>
>>>> Linux (RHEL7.7):
>>>>
>>>> # cat hs_err_pid251046.log |grep microc
>>>>
>>>> CPU: total 112 (initial active 112) (28 cores per cpu, 2 threads per
>>>> core) family 6 model 85 stepping 4 microcode 0x200005e, cmov, cx8,
>>>> fxsr, mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt,
>>>> vzeroupper, avx, avx2, aes, clmul, erms, rtm, 3dnowpref, lzcnt, ht,
>>>> tsc, tscinvbit, bmi1, bmi2, adx, fma, clflush, clflushopt, clwb
>>>>
>>>>
>>>>
>>>> Windows (Win10, v1809):
>>>>
>>>> CPU: total 4 (initial active 4) (2 cores per cpu, 2 threads per
>>>> core) family 6 model 142 stepping 9 microcode 0xb4, cmov, cx8, fxsr,
>>>> mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper,
>>>> avx, avx2, aes, clmul, erms, rtm, 3dnowpref, lzcnt, ht, tsc,
>>>> tscinvbit, bmi1, bmi2, adx, fma, clflush, clflushopt
>>>>
>>>>
>>>>
>>>> MacOS (Darwin):
>>>>
>>>> $ cat hs_err_pid95187.log |grep microc
>>>>
>>>> CPU: total 8 (initial active 8) (4 cores per cpu, 2 threads per
>>>> core) family 6 model 126 stepping 5 microcode 0x78, cmov, cx8, fxsr,
>>>> mmx, sse, sse2, sse3, ssse3, sse4.1, sse4.2, popcnt, vzeroupper,
>>>> avx, avx2, aes, clmul, erms, 3dnowpref, lzcnt, ht, tsc, tscinvbit,
>>>> bmi1, bmi2, adx, sha, fma, clflush, clflushopt
>>>>
>>>>
>>>>
>>>> Thanks, Vladimir
>>>>
>>>>
>>>>     Thanks, Vladimir
>>>>
>>>>


More information about the hotspot-runtime-dev mailing list