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

Wed Mar 9 12:19:31 UTC 2016

Hi Thomas,

Thanks for identifying the issue. I will file a new bug for this and correct the issue.

Regards,

Cheleswer

From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] 
Sent: Wednesday, March 09, 2016 4:44 PM
To: David Holmes
Cc: Kevin Walls; Cheleswer Sahu; Daniel Daugherty; 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

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 <HYPERLINK "mailto:david.holmes at oracle.com"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: HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net;
HYPERLINK "mailto:hotspot-runtime-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
<HYPERLINK "mailto:cheleswer.sahu at oracle.com"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
<HYPERLINK "mailto:cheleswer.sahu at oracle.com"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/3b742959/attachment-0001.html>