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.hol...@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@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 >>>>>>> >>>>>>> >>> >>