RFR(S) 8241004: NMT tests fail on unalinged thread size with debug build

[Dmitry Samersoff]

Hello Boris,

Looks good to me.

-Dmitry

On 19.05.2020 20:55, Boris Ulasevich wrote:
> Hi Zhengyu,
> 
> On 18.05.2020 19:00, Zhengyu Gu wrote:
>> Hi Boris,
>>
>> I could not find a system to reproduce, I am not sure how it works.
>>
>> So, I have a few questions:
>>
>>>
>> -  assert(is_aligned(start, page_sz), "Start address must be page 
>> aligned");
>> -  assert(is_aligned(size, page_sz), "Size must be page aligned");
>> +  address aligned_start = align_down(start, page_sz);
>> +  address aligned_end   = align_up(start + size, page_sz);
>> +  int     page_num      = (aligned_end - aligned_start) / page_sz;
>>
>> Below start address should be stack guard pages. IIRC, stack guards 
>> are page aligned, right? so how can start address not page aligned?
>>
>> Align down start address seems can collide guard pages?
> 
> Yes, Hotspot guard page is just below start address, and start address 
> is page aligned in my case.
> 
> The start address change was done in webrev.01 to make the input range 
> completely unrestricted. The original webrev.00 had the modification for 
> size only (see this mail thread).
> 
>>> I'm not sure I got the idea.VirtualMemoryTracker is already doing 
>>> what you said: "scanning reserved regions for committed regions":
>>> - snapshot_thread_stacks() iterates over reserved regions
>>> - for each mtThreadStack region SnapshotThreadStackWalker iterates 
>>> over its committed ranges
>>> - committed ranges of a given region are provided by linux kernel and 
>>> summed up by ReservedMemoryRegion::add_committed_region
>>
>> If unaligned stack is due to unaligned base (stack top), would 
>> aligning up stack size in 
>> SnapshotThreadStackWalker::do_allocation_site() solve the problem?
> 
> Yes, with a fix for "Not contain this region" assertion [1] it can be 
> done this way:
> http://cr.openjdk.java.net/~bulasevich/8241004/webrev.02b
> 
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/a44396903fd8/src/hotspot/share/services/virtualMemoryTracker.cpp#l112 
> 
> 
>> Thanks,
>>
>> -Zhengyu
>>
>>>
>>> Yes, it is unaligned thread stack issue reproduced with 
>>> -XX:NativeMemoryTracking option.
>>>
>>>> -Zhengyu
>>>>
>>>> [1] 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/a44396903fd8/src/hotspot/share/services/virtualMemoryTracker.cpp#l531 
>>>>
>>>>
>>>>>
>>>>> Thanks, Thomas
>>>>>
>>>>>
>>>>>
>>>>> On Wed, May 13, 2020 at 6:29 PM Boris Ulasevich 
>>>>> <boris.ulasevich at bell-sw.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Thomas,
>>>>>>
>>>>>> Good point! Thanks.
>>>>>> I reworked the function to ease both restrictions.
>>>>>> http://cr.openjdk.java.net/~bulasevich/8241004/webrev.01
>>>>>>
>>>>>> regards,
>>>>>> Boris
>>>>>>
>>>>>> On 13.05.2020 08:59, Thomas Stüfe wrote:
>>>>>>
>>>>>> Hi Boris,
>>>>>>
>>>>>> Note that we have unaligned stack boundaries on AIX too, not so 
>>>>>> strange :)
>>>>>>
>>>>>> What I do not like about the solution is that it loosens the 
>>>>>> restriction
>>>>>> on the input size while keeping the restriction on the start 
>>>>>> address. I'd
>>>>>> rather see this function either in its current form (requiring the 
>>>>>> input
>>>>>> ranges to be page aligned) or the input range to be completely
>>>>>> unrestricted. This function is currently only used by NMT but it 
>>>>>> is useful
>>>>>> and may be used in other places too.
>>>>>>
>>>>>> Can you make it work with arbitrary input ranges? I do not think this
>>>>>> would require much changes from your patch, you do most of the 
>>>>>> work already.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> ..Thomas
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Thu, May 7, 2020 at 2:56 PM boris <boris.ulasevich at bell-sw.com> 
>>>>>> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the following change.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~bulasevich/8241004/webrev.00
>>>>>>> http://bugs.openjdk.java.net/browse/JDK-8241004
>>>>>>>
>>>>>>> The stack size created by the implementation of the pthread 
>>>>>>> library is
>>>>>>> not necessarily page aligned. It is weird, but it is not 
>>>>>>> forbidden: the
>>>>>>> pthread library may add some gap on it own. My change removes 
>>>>>>> stack size
>>>>>>> alignment assert checks and tunes NMT committed range calculation 
>>>>>>> for
>>>>>>> the case of unaligned stack size.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Boris
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>