RFR[9u-dev]: 8146683: check_addr0 should be more efficient

[Kevin Walls]

Hi Cheleswer,

Looks good.  (While we've discussed it offline I haven't actually 
offered a public review).

Just one more thing, and this concerns the original code not the change:

1880           st->print("Warning: Address: 0x%x, Size: %dK, 
",p->pr_vaddr, p->pr_size/1024, p->pr_mapname);

We pass 3 params when we only print two items?  The "p->pr_mapname", we 
pass again on the next line where we do have the the "%s" in the format 
string, so this one is unnecessary. 8-)

Thanks
Kevin



On 07/03/2016 10:07, Dmitry Samersoff wrote:
> Cheleswer,
>
> Looks good for me.
>
> -Dmitry
>
>
> On 2016-03-03 19:28, Cheleswer Sahu wrote:
>> Hi,
>>
>> Please review the new code changes for this bug. I have removed  " fstat()"  call which seems not safe to read the "/proc/map/self" file and have made some other changes too. Please find the latest code in below link
>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/webrev.01/
>>
>>
>> Regards,
>> Cheleswer
>>
>> -----Original Message-----
>> From: Dean Long
>> Sent: Wednesday, February 24, 2016 7:44 AM
>> To: Daniel Daugherty; Thomas Stüfe; Cheleswer Sahu
>> Cc: serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR[9u-dev]: 8146683: check_addr0 should be more efficient
>>
>> On 2/23/2016 10:07 AM, Daniel D. Daugherty wrote:
>>> On 2/23/16 5:56 AM, Thomas Stüfe wrote:
>>>> Hi Cheleswer,
>>>>
>>>>
>>>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu
>>>> <cheleswer.sahu at oracle.com>
>>>>    wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> Please review the code changes for
>>>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>>>
>>>>>
>>>>>
>>>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>>>
>>>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>>>
>>>>>
>>>>>
>>>>> Bug Brief:
>>>>>
>>>>> While investigating  bug
>>>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>>>> it has been observed that "check_addr0() " function  is not written
>>>>> efficiently.
>>>>>
>>>>> This function is trying to read each "prmap_t" entry in
>>>>> "/proc/self/map"
>>>>> file one by one which is time taking and can cause in long pauses.
>>>>>
>>>>>
>>>>>
>>>>> Solution proposed:
>>>>>
>>>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one
>>>>> by one, read the entries in chunks. There can be many entries in "map"
>>>>> file,
>>>>> so I have decided to read these
>>>>>
>>>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves
>>>>> a lot of read calls and results in smaller pause times.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Cheleswer
>>>>>
>>>> We saw cases, especially on Solaris, where the content of a file in the
>>>> proc file system changed under us while we were reading it. So this
>>>> should
>>>> be kept in mind. The original code seems also broken in that regard,
>>>> because it assumes the ::read() call to read the full size of a prmap_t
>>>> entry, but it may theoretically read an incomplete entry.
>>>>
>>>> As for your coding, you first estimate the size of the mapping file and
>>>> then use this to determine how many entries to read; but when
>>>> reading, this
>>>> information may already be stale. I think it would be more robust and
>>>> also
>>>> simpler to just try to read as many entries as possible - in chunks, to
>>>> make reading faster - until you get EOF.
>>>>
>>>> Kind Regards, Thomas
>>> I'm really sure that we've been down this road before. In particular,
>>> Dmitry Samersoff has worked on this issue (or one very much like it)
>>> before, but he is on vacation until the end of the month.
>>>
>> There was a similar issue with Linux /proc/self/maps, and whether it was
>> safe to read the file with buffered stdio or not.  See 8009062 and 7017193.
>>
>> dl
>>
>>> Dan
>>>
>>>
>>>> On Tue, Feb 23, 2016 at 9:38 AM, Cheleswer Sahu
>>>> <cheleswer.sahu at oracle.com>
>>>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> Please review the code changes for
>>>>> https://bugs.openjdk.java.net/browse/JDK-8146683 .
>>>>>
>>>>>
>>>>>
>>>>> Webrev link: http://cr.openjdk.java.net/~csahu/8146683/
>>>>>
>>>>> JBS Link: https://bugs.openjdk.java.net/browse/JDK-8146683
>>>>>
>>>>>
>>>>>
>>>>> Bug Brief:
>>>>>
>>>>> While investigating  bug
>>>>> https://bugs.openjdk.java.net/browse/JDK-8144483
>>>>> it has been observed that "check_addr0() " function  is not written
>>>>> efficiently.
>>>>>
>>>>> This function is trying to read each "prmap_t" entry in
>>>>> "/proc/self/map"
>>>>> file one by one which is time taking and can cause in long pauses.
>>>>>
>>>>>
>>>>>
>>>>> Solution proposed:
>>>>>
>>>>> Instead of reading each "prmap_t" entry in "/proc/self/map" file one by
>>>>> one, read the entries in chunks. There can be many entries in "map"
>>>>> file,
>>>>> so I have decided to read these
>>>>>
>>>>> in chunks of 100  "prmap_t" entries. Reading entries in chunks saves
>>>>> a lot
>>>>> of read calls and results in smaller pause times.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Cheleswer
>>>>>
>