CPU-feature detection is broken for new Fujitsu Sparc64-X CPUs

[Vladimir Kozlov]

 >>>     if (is_niagara() || (is_sun4v() && is_sparc64())) {
 > So I think the above check is the best I can currently do. But I will be
 > happy to change it once we get new informations.

I think we should simple use has_blk_init() check to guard 
UseMemSetInBOT setting.

Regards,
Vladimir

On 11/27/13 12:43 PM, Volker Simonis wrote:
> On Wednesday, November 27, 2013, Vladimir Kozlov wrote:
>
>> On 11/27/13 10:02 AM, Volker Simonis wrote:
>>
>>> Hi Vladimir,
>>>
>>> thanks a lot for commenting on this issue in the bug report.
>>>
>>>
>>> - Regarding 'UseMemSetInBOT' I agree that we should know how memset() is
>>> implemented on Solaris/Fujitsu Sparc-X and/or how BIS is working on
>>> Fujitsu
>>> Sparc-X. Could you please find this out. I don't see how I could get this
>>> information:(
>>>
>>
>> I will try :)
>>
>>
>>> But in order to be on the safe side, I think we should reset
>>> UseMemSetInBOT
>>> until then:
>>>
>>>     if (is_niagara() || (is_sun4v() && is_sparc64())) {
>>>       // When using CMS or G1, we cannot use memset() in BOT updates
>>>       // because the sun4v/CMT version in libc_psr uses BIS which
>>>       // exposes "phantom zeros" to concurrent readers. See 6948537.
>>>       if (FLAG_IS_DEFAULT(UseMemSetInBOT) && (UseConcMarkSweepGC ||
>>> UseG1GC))
>>> {
>>>         FLAG_SET_DEFAULT(UseMemSetInBOT, false);
>>>       }
>>>
>>> Currently, this is not done, that's why the second assertion fires. But
>>> you
>>> don't see it in a product build and this really bothers me most because I
>>> don't want to run into these problems on new Fujitsu machines. What do you
>>> think?
>>>
>>
>> Totally agree. Why you need 'is_sun4v() &&' ? Can we have sparc64 which
>> are not sun4v?
>
>
> Actually I simply don't know. From Wikipedia (
> http://en.wikipedia.org/wiki/SPARC64_VI) I read that there exist Sparc64 V,
> VI, VII, VIII, IX and X, but I don't know which of them is the first that
> implemented sun4v. We only recently got a Sparc64-X so that's the only one
> I can actually look at.
>
> So I think the above check is the best I can currently do. But I will be
> happy to change it once we get new informations.
>
>
>>   - Regarding 'is_niagara(int features)', would it be fine if I'd change:
>>>
>>> static bool is_niagara(int features)  { return (features & sun4v_m) != 0;
>>> }
>>>
>>> to:
>>>
>>> static bool is_niagara(int features)  { return (features & sun4v_m) != 0
>>> &&
>>> (features & sparc64_family_m) == 0; }
>>>
>>
>> I prefer this change. Add comment that sun4v_m is true on both.
>
>
> OK, I'll prepare a webrev.
>
>
>>
>>
>>> With this change, I could leave the assertion:
>>>
>>>     assert((is_T_family(features) == is_niagara(features)), "Niagara should
>>> be T series");
>>>
>>> untouched and it would work on both, Oracle and Fujitsu CPUs.
>>>
>>> On the other hand, maybe it would be clearer to remove
>>> is_niagara(features)
>>> altogether and replace create two new function:
>>>
>>>     static bool is_sparc64(int features)              { return (features &
>>> sparc64_family_m) != 0; }
>>>     static bool is_sun4v(int features)                { return (features &
>>> sun4v_m) != 0; }
>>>
>>> Then we could change the assertion to:
>>>
>>>     assert((is_T_family(features) == is_sun4v(features) ||
>>> is_sparc64(features)), "Niagara should be T series");
>>>
>>> What do you think?
>>>
>>>
>>>
>>> - Regarding UseBlockCopy and UseBlockZeroing: what do you mean by "'block
>>> init instructions' could be implemented differently on Fujitsu Sparc-X"?
>>> Currently we only check 'has_block_zeroing()' for both options, but
>>> 'has_block_zeroing()' only returns true on T4. On the other hand, Fujitsu
>>> Sparc64-X supports the 'blk_init' extension - it's just not clear if it
>>> works like for T1/T2 or like for T4. So depending on this answer we should
>>> change 'has_block_zeroing()' to also work correctly on Sparc64-X, right?
>>>
>>
>> BIS was since T1. But T4 implementation of BIS instructions GUARANTEE
>> zeroing of cacheline before storing value (STXA) without fetching data from
>> memory. It is used only for big arrays (BlockZeroingLowLimit) because it
>> still need StoreLoad membar (which is expensive). See
>> MacroAssembler::bis_zeroing(). It used for initializing new arrays before
>> they are visible to other threads since the state of memory before membar
>> is undefined.
>>
>> And I don't know how particular STXA ASI_ST_BLKINIT_PRIMARY works on
>> Sparc64-X.
>
>
> Who could we ask? Maybe the Solaris-Kernel people (unfortunately there's no
> more OpenSolaris)? Maybe some Fujitsu folks?
>
>
>>
>> Regards,
>> Vladimir
>>
>>
>>
>>
>> On Tue, Nov 26, 2013 at 6:35 PM, Volker Simonis <volker.simonis at gmail.com
>>> wrote:
>>
>>   Hi,
>>
>> I've just opened "JDK-8029190: VM_Version::determine_features()
>> asserts on Fujitsu Sparc64 CPUs"
>> (https://bugs.openjdk.java.net/browse/JDK-8029190) and would like to
>> get the input of some real SPARC experts on how this could be best
>> fixed:
>>
>> The whole CPU detection for SPARC CPUs seems to be targeted solely to
>> Sun/Oracle SPARC CPUs.
>>
>> If running on a recent Fujitsu Sparc64-X CPU, debug builds immediately
>> crash with:
>>
>>    assert(is_T_family(features) == is_niagara(features)) failed: Niagara
>> should be T series
>>
>> This test can not be successful on a Fujitsu Sparc64 CPU because
>> is_niagara(features) tests for the sysinfo string 'SI_MACHINE' which
>> is 'sun4v' on both, Oracle and Fujitsu CPUs while
>> is_T_family(features) checks that the kernel statistics module
>> 'cpu_info.implementation' contains the name "SPARC-T" or "SPARC-M".
>> However, on Fujitsu SPARC64-X CPUs, 'cpu_info.implementation' contains
>> the value "SPARC64-X".
>>
>> This problem can easily be worked around by changing the above assertion
>> to:
>>
>> assert((is_T_family(features) == is_niagara(features)) || (features &
>> sparc64_family_m), "Niagara should be T series");
>>
>> But if we are using a CMS or G1, the VM will immediately crash with
>> the next assertion in arguments.cpp:
>>
>>       if (VM_Version::is_sun4v() && UseMemSetInBOT) {
>>         assert(!FLAG_IS_DEFAULT(UseMemSetInBOT), "Error");
>>
>> This is again because 'is_sun4v()' is used with the semantics of
>> 'is_niagara()' and in VM_Version::initialize() in vm_version_sparc.cpp
>> we reset 'UseMemSetInBOT' to false if 'is_niagara()' is true:
>>
>>     if (is_niagara()) {
>>       // When using CMS or G1, we cannot use memset() in BOT updates
>>       // because the sun4v/CMT version in libc_psr uses BIS which
>>       // exposes "phantom zeros" to concurrent readers. See 6948537.
>>       if (FLAG_IS_DEFAULT(UseMemSetInBOT) && (UseConcMarkSweepGC ||
>> UseG1GC)) {
>>         FLAG_SET_DEFAULT(UseMemSetInBOT, false);
>>       }
>>
>> We could actually fix this in the same way like before, by replacing
>> 'is_sun4v()' with 'is_niagara()' in the assertion, but this would
>> require to make at least the following functions public in
>> vm_version_sparc.hpp:
>>
>>     static bool is_T_family(int features) { return (features & T_family_m)
>> != 0; }
>>     static bool is_niagara() { return is_T_family(_features); }
>>
>> However the bigger problem is that I have no idea of what the
>> semantics of these attributes (i.e. is_niagara, is_T_family,
>> is_M_family) is for Fujitsu Sparc-X CPUs? E.g. do we need to switch of
>> 'UseMemSetInBOT' as well?
>>
>> There's are also other CPU-features like 'UseBlockCopy' and
>> 'UseBlockZeroing' which are actually detected by looking at the
>> instruction set extensions AND the CPU family. E.g. the above options
>> are only enabled if the CPU supports 'block init instructions' (which
>> is true on both Fujitsu Sparc64-X and new Oracle Sparc-T4 CPUs) AND
>> the CPU family is Sparc-T4 (which is of course only true for Oracle
>> SParc CPUs).
>>
>> Altogether, I think we need a real SPARC expert which knows both, the
>> Oracle and the Fujitsu Sparc CPUs and who can tell us which of the
>> Oracle Sparc features currently used in the HotSpot are available in
>> Fujitsu CPUs as well. Moreover we should unify the various 'has_xxx()'
>> and 'is_xxx()' functions from vm_version_sparc.hpp to work equally
>> well for both CPU types.
>>
>> Thank you and best regards,
>> Volker
>>
>> PS:
>>
>> - the Fujitsu Sparc64-X I have access to returns the following
>> instruction set extensions vector and CPU features:
>>
>> getisax(2) returned: 0x400081f0
>> cpu_info.implementation: SPARC64-X (chipid 0, clock 2800 MHz)
>> Allocation prefetching: PREFETCH at distance 512, 3 lines of 64 bytes
>> PrefetchCopyIntervalInBytes 512
>> PrefetchScanIntervalInBytes 512
>> ContendedPaddingWidth 128
>> CPU:total 16 v9, popc, vis1, vis2, blk_init, sun4v, sparc64
>>
>> - for the Sparc-T4 I've acess to this information looks as follows:
>>
>> getisax(2) returned: 0x3ffe8df0
>> cpu_info.implementation: SPARC-T4 (chipid 0, clock 2848 MHz)
>> Allocation prefetching: BIS at distance 64, 6 lines of 32 bytes
>> PrefetchCopyIntervalInBytes 512
>> Prefe
>>
>>