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@openjdk.java.net; hotspot-runtime-...@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.s...@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.s...@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 >>> >