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

[Thomas Stüfe]

Hi all,

I see the change is already pushed, but I am not sure this works as
intended:

We iterate now over all fully read items:

+      p = (prmap_t *)mbuff;
+      for(int i = 0; i < nmap; i++){
+        if (p->pr_vaddr == 0x0) {
+   ....
+        }
+        p = (prmap_t *)(mbuff + sizeof(prmap_t));
+      }

Where do we move the pointer forward? The way I see it, we only ever
examine the first and the second item, then repeat examining the second
item over and over again.

Instead of

+        p = (prmap_t *)(mbuff + sizeof(prmap_t));

Should this not be

p = ((prmap_t *)mbuff) + i;

or just

p++;

?

Kind Regards, Thomas






On Mon, Mar 7, 2016 at 12:23 PM, David Holmes <david.holmes at oracle.com>
wrote:

> 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
>>>>>>>
>>>>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160309/00103ed4/attachment-0001.html>