Re: [PATCH] Let gcore use ptrace interface rather than the procfs
Robert Watson
> This patch allows gcore to use the ptrace interface rather than procfs:
> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore.diff
>
> The main visible effect of that is that gcore can now work on a per-thread
> scope, offering a granularity procfs can't reach. A downside, though, is
> that the process to be targeted is going to be stopped with ptrace. This
> patch has been contributed back by Sandvine Incorporated. Comments, reviews
> and testing are welcome.
Am I right in thinking that this may run into a number of other issues that
the procfs version didn't:
- gcore may no longer work on processes that are actively being debugged with
gdb or traced with truss.
- gcore may cause interruptible system calls in the target process to return
EINTR, and interfere with signal delivery.
If so, these aren't show-stoppers, but we should make sure they're documented
in the gcore man page. Fixing gcore would be excellent, it got missed in the
initial sweep of things broken by disabling procfs by default.
Thanks for doing this work,
Robert N M Watson
Computer Laboratory
University of Cambridge
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-curre...@freebsd.org"
Ed Maste
> On Mon, 16 Nov 2009, Attilio Rao wrote:
>
> >This patch allows gcore to use the ptrace interface rather than procfs:
> >http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore.diff
> >
> >The main visible effect of that is that gcore can now work on a per-thread
> >scope, offering a granularity procfs can't reach. A downside, though, is
> >that the process to be targeted is going to be stopped with ptrace. This
> >patch has been contributed back by Sandvine Incorporated. Comments,
> >reviews and testing are welcome.
>
> Am I right in thinking that this may run into a number of other issues that
> the procfs version didn't:
>
> - gcore may no longer work on processes that are actively being debugged
> with gdb or traced with truss.
>
> - gcore may cause interruptible system calls in the target process to
> return EINTR, and interfere with signal delivery.
>
> If so, these aren't show-stoppers, but we should make sure they're
> documented in the gcore man page. Fixing gcore would be excellent, it got
> missed in the initial sweep of things broken by disabling procfs by default.
Our original motivation for doing this was to make gcore work with
threaded apps, not avoiding procfs, but that's a useful side-effect of
the work. Note though that for that purpose it isn't complete; procfs
is still used in readmap to read the process' memory map. It looks like
we need to find a way to implement readmap without procfs.
Thanks,
Ed
Robert N. M. Watson
On 17 Nov 2009, at 14:17, Ed Maste wrote:
> Our original motivation for doing this was to make gcore work with
> threaded apps, not avoiding procfs, but that's a useful side-effect of
> the work. Note though that for that purpose it isn't complete; procfs
> is still used in readmap to read the process' memory map. It looks like
> we need to find a way to implement readmap without procfs.
Are the sysctls used for procstat -v sufficient for this purpose?
Robert
Attilio Rao
>
> On 17 Nov 2009, at 14:17, Ed Maste wrote:
>
>> Our original motivation for doing this was to make gcore work with
>> threaded apps, not avoiding procfs, but that's a useful side-effect of
>> the work. Note though that for that purpose it isn't complete; procfs
>> is still used in readmap to read the process' memory map. It looks like
>> we need to find a way to implement readmap without procfs.
>
> Are the sysctls used for procstat -v sufficient for this purpose?
This patch should address the arised concerns by both of you:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore2.diff
and additively fix elf_getstatus() to not use procfs, so that gcore is
completely procfs independent now.
Comments, reviews and testing are welcome.
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
Robert Watson
On Thu, 19 Nov 2009, Attilio Rao wrote:
> 2009/11/17 Robert N. M. Watson <rwa...@freebsd.org>:
>>
>> On 17 Nov 2009, at 14:17, Ed Maste wrote:
>>
>>> Our original motivation for doing this was to make gcore work with
>>> threaded apps, not avoiding procfs, but that's a useful side-effect of the
>>> work. Note though that for that purpose it isn't complete; procfs is
>>> still used in readmap to read the process' memory map. It looks like we
>>> need to find a way to implement readmap without procfs.
>>
>> Are the sysctls used for procstat -v sufficient for this purpose?
>
> This patch should address the arised concerns by both of you:
> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore2.diff
>
> and additively fix elf_getstatus() to not use procfs, so that gcore is
> completely procfs independent now. Comments, reviews and testing are
> welcome.
If you add the missing include of sys/wait.h, elfcore.c generates an error
instead of a warning on this non-traditional use of wait(2):
+ wait();
Something like this may be preferred:
if (waitpid(pid, NULL, 0) < 0)
err(1, "waitpid");
This further persisting reference to procfs can be replaced with a sysctl/kvm
interface:
gcore.c: asprintf(&binfile, "/proc/%d/file", pid);
See the implementation of "procstat -b" which returns the path to the binary
using the same underlying mechanism (vn_fullpath on the process image vnode).
I think that kills the last of the procfs dependencies, in which case perhaps
we can remove the procfs.h include from elfcore.c, which requires defining a
local version of a summary data structure borrowed from procfs. It's worth
trying with procfs unmounted, however, to make sure they're really all gone
(which is how I ran into the above problem).
Robert N M Watson
Computer Laboratory
University of Cambridge
_______________________________________________
Attilio Rao
>
> On Thu, 19 Nov 2009, Attilio Rao wrote:
>
>> 2009/11/17 Robert N. M. Watson <rwa...@freebsd.org>:
>>>
>>> On 17 Nov 2009, at 14:17, Ed Maste wrote:
>>>
>>>> Our original motivation for doing this was to make gcore work with
>>>> threaded apps, not avoiding procfs, but that's a useful side-effect of the
>>>> work. Note though that for that purpose it isn't complete; procfs is still
>>>> used in readmap to read the process' memory map. It looks like we need to
>>>> find a way to implement readmap without procfs.
>>>
>>> Are the sysctls used for procstat -v sufficient for this purpose?
>>
>> This patch should address the arised concerns by both of you:
>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore2.diff
>>
>> and additively fix elf_getstatus() to not use procfs, so that gcore is
>> completely procfs independent now. Comments, reviews and testing are
>> welcome.
>
> If you add the missing include of sys/wait.h, elfcore.c generates an error
> instead of a warning on this non-traditional use of wait(2):
>
> + wait();
>
> Something like this may be preferred:
>
> if (waitpid(pid, NULL, 0) < 0)
> err(1, "waitpid");
I didn't get a warning neither an error but yes, the waitpid() is
preferred and should be used.
> This further persisting reference to procfs can be replaced with a
> sysctl/kvm interface:
>
> gcore.c: asprintf(&binfile, "/proc/%d/file", pid);
Right, I just modified elfcore so I missed it.
> See the implementation of "procstat -b" which returns the path to the binary
> using the same underlying mechanism (vn_fullpath on the process image
> vnode).
>
> I think that kills the last of the procfs dependencies, in which case
> perhaps we can remove the procfs.h include from elfcore.c, which requires
> defining a local version of a summary data structure borrowed from procfs.
> It's worth trying with procfs unmounted, however, to make sure they're
> really all gone (which is how I ran into the above problem).
I don't like the idea to replicate the structures because of code
maintence. IMHO is ok to have procfs header.
I will provide ASAP a new patch which addresses this concerns and
testing without procfs mounted.
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
Robert N. M. Watson
On 19 Nov 2009, at 13:45, Attilio Rao wrote:
>> If you add the missing include of sys/wait.h, elfcore.c generates an error
>> instead of a warning on this non-traditional use of wait(2):
>>
>> + wait();
>>
>> Something like this may be preferred:
>>
>> if (waitpid(pid, NULL, 0) < 0)
>> err(1, "waitpid");
>
> I didn't get a warning neither an error but yes, the waitpid() is
> preferred and should be used.
This warning was on i386 9.x, FYI, and was a property of failing to call wait(2) with an argument.
>> I think that kills the last of the procfs dependencies, in which case
>> perhaps we can remove the procfs.h include from elfcore.c, which requires
>> defining a local version of a summary data structure borrowed from procfs.
>> It's worth trying with procfs unmounted, however, to make sure they're
>> really all gone (which is how I ran into the above problem).
>
> I don't like the idea to replicate the structures because of code
> maintence. IMHO is ok to have procfs header.
I'm not sure I agree; looking at the elfcore code, it looks like it goes to some amount of inconvenience to stuff things into the structure in the first place, primarily because that was how procfs exported it. With your excellent change, there's no need for gcore(1) to depend on procfs-specific data structures that may change, or more ideally, be removed in the future.
Robert_______________________________________________
Attilio Rao
>
> On 19 Nov 2009, at 13:45, Attilio Rao wrote:
>
>>> If you add the missing include of sys/wait.h, elfcore.c generates an error
>>> instead of a warning on this non-traditional use of wait(2):
>>>
>>> + wait();
>>>
>>> Something like this may be preferred:
>>>
>>> if (waitpid(pid, NULL, 0) < 0)
>>> err(1, "waitpid");
>>
>> I didn't get a warning neither an error but yes, the waitpid() is
>> preferred and should be used.
>
> This warning was on i386 9.x, FYI, and was a property of failing to call wait(2) with an argument.
>
>>> I think that kills the last of the procfs dependencies, in which case
>>> perhaps we can remove the procfs.h include from elfcore.c, which requires
>>> defining a local version of a summary data structure borrowed from procfs.
>>> It's worth trying with procfs unmounted, however, to make sure they're
>>> really all gone (which is how I ran into the above problem).
>>
>> I don't like the idea to replicate the structures because of code
>> maintence. IMHO is ok to have procfs header.
>
>
> I'm not sure I agree; looking at the elfcore code, it looks like it goes to some amount of inconvenience to stuff things into the structure in the first place, primarily because that was how procfs exported it. With your excellent change, there's no need for gcore(1) to depend on procfs-specific data structures that may change, or more ideally, be removed in the future.
Yeah, I had the same feeling as the interfaces should be more lifted
in order to less fit procfs (example: probabilly readmap could export
directly the list of objects from libutil rather then transforming it)
but let's get there in a second round of changes probabilly.
Thanks,
Attilio
--
Peace can only be achieved by understanding - A. Einstein
_______________________________________________
Attilio Rao
> 2009/11/19 Robert N. M. Watson <rwa...@freebsd.org>:
>>
>> On 19 Nov 2009, at 13:45, Attilio Rao wrote:
>>
>>>> If you add the missing include of sys/wait.h, elfcore.c generates an error
>>>> instead of a warning on this non-traditional use of wait(2):
>>>>
>>>> + wait();
>>>>
>>>> Something like this may be preferred:
>>>>
>>>> if (waitpid(pid, NULL, 0) < 0)
>>>> err(1, "waitpid");
>>>
>>> I didn't get a warning neither an error but yes, the waitpid() is
>>> preferred and should be used.
>>
>> This warning was on i386 9.x, FYI, and was a property of failing to call wait(2) with an argument.
>>
>>>> I think that kills the last of the procfs dependencies, in which case
>>>> perhaps we can remove the procfs.h include from elfcore.c, which requires
>>>> defining a local version of a summary data structure borrowed from procfs.
>>>> It's worth trying with procfs unmounted, however, to make sure they're
>>>> really all gone (which is how I ran into the above problem).
>>>
>>> I don't like the idea to replicate the structures because of code
>>> maintence. IMHO is ok to have procfs header.
>>
>>
>> I'm not sure I agree; looking at the elfcore code, it looks like it goes to some amount of inconvenience to stuff things into the structure in the first place, primarily because that was how procfs exported it. With your excellent change, there's no need for gcore(1) to depend on procfs-specific data structures that may change, or more ideally, be removed in the future.
>
> Yeah, I had the same feeling as the interfaces should be more lifted
> in order to less fit procfs (example: probabilly readmap could export
> directly the list of objects from libutil rather then transforming it)
> but let's get there in a second round of changes probabilly.
This further patch should address the last reported issues and a
couple of style nits:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/gcore/gcore3.diff
Please also note that I added the Sandvine's copyright because I think
the extent of the changes deserve it.