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

[David Holmes]

On 7/03/2016 8:36 PM, Kevin Walls wrote:
>
> 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);

Also %x is the wrong format specifier:

typedef struct prmap {
	uintptr_t pr_vaddr;	/* virtual address of mapping */

David
-----

> 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
>>>>>>
>>
>