Fix procstat kvm_read issues

[dougm]

Summary

procstat fails because we don't kvm_read every entry as part of the search for the successor. Define a vm_map method to allow advance from an entry to its successor using a reader method and a token passed in along with the entry. Use that method in procstat.

Note that, currently, procstat_getfiles_kvm goes into an infinite loop if kvm_read_all fails to read a map entry, so I'm trying to address that at the same time.

[dougm]

Fix compilation errors.

Test:

:/local/obj/freebsd/dougm/head/amd64.amd64/usr.bin/procstat # ./procstat /home/dougm/jnk.core

PID  PPID  PGID   SID  TSID THR LOGIN    WCHAN     EMUL          COMM

29493 71354 29493 71354 71354 1 dougm - FreeBSD ELF64 jnk

[markj]

I don't have an objection to this approach, but it's kind of unusual when compared with other routines which use libkvm. That is, I would just implement entry_read_succ directly in libprocstat. With this patch, libprocstat still embed details of the implementation (such as the fact that iteration terminates after visiting the header) so we still do not get a clean separation between userspace and the kernel, and vm_map_entry_read_succ() is harder to understand because of the indirection through "reader".

sys/vm/vm_map.h

409 ↗

(On Diff #65400)

I think this should have a comment explaining what it's for.

[dougm]

In D22728#497468, @markj wrote:

I don't have an objection to this approach, but it's kind of unusual when compared with other routines which use libkvm. That is, I would just implement entry_read_succ directly in libprocstat. With this patch, libprocstat still embed details of the implementation (such as the fact that iteration terminates after visiting the header) so we still do not get a clean separation between userspace and the kernel, and vm_map_entry_read_succ() is harder to understand because of the indirection through "reader".

I don't want the implement of threaded trees leaking out of vm_map.[ch]. I probably should have not inlined it, and put it in vm_map.c. I don't want to bring kvm into vm_map.[ch], so 'reader' seems the best separation I can manage. I could define start and end functions to more clearly separate 'header' from libprocstat.

[markj]

In D22728#497472, @dougm wrote:

In D22728#497468, @markj wrote:

I don't have an objection to this approach, but it's kind of unusual when compared with other routines which use libkvm. That is, I would just implement entry_read_succ directly in libprocstat. With this patch, libprocstat still embed details of the implementation (such as the fact that iteration terminates after visiting the header) so we still do not get a clean separation between userspace and the kernel, and vm_map_entry_read_succ() is harder to understand because of the indirection through "reader".

I don't want the implement of threaded trees leaking out of vm_map.[ch]. I probably should have not inlined it, and put it in vm_map.c. I don't want to bring kvm into vm_map.[ch], so 'reader' seems the best separation I can manage. I could define start and end functions to more clearly separate 'header' from libprocstat.

It is basically impossible to achieve a clean separation due to the nature of libkvm-based accesses. I think this approach is fine, just so long as it's clearly indicated what vm_map_entry_read_succ() is for.

[dougm]

Add comment. Change vm_map_entry_read_succ so that it leaves the next entry in vmentry, and doesn't only return the pointer to that entry.

Waiting for compile and retest now.

[kib]

I was bitten by use of seemingly kernel-only headers in userspace too many times. I believe that it is much better to not try to adapt vm/vm_map.h inlines to userspace, but have separate code in libprocstat, as Mark suggested.

If committing this patch, you need to specify that consumer of vm_map_entry_read_succ() is in userspace, otherwise grep in sys/ would prompt somebody to remove the function.

[dougm]

In D22728#497480, @kib wrote:

I was bitten by use of seemingly kernel-only headers in userspace too many times. I believe that it is much better to not try to adapt vm/vm_map.h inlines to userspace, but have separate code in libprocstat, as Mark suggested.

This weekend panic is because, when I tried to arrange everything so that the last changes were to vm_map.[ch] only, I missed this libprocstat stuff in user space. If, next week, someone decides to re-introduce the next and prev pointers, I'd like them to have to change only vm_map.[ch] and not go hunting for libprocstat.c. I don't think burying the implementation in libprocstat.c makes sense.

If committing this patch, you need to specify that consumer of vm_map_entry_read_succ() is in userspace, otherwise grep in sys/ would prompt somebody to remove the function.

I'll add more comment. The fact that it's not in the #defined _KERNEL part already says that it's userspace-visible, but I'll add more comment anyway.

[dougm]

Make more clear that the new function is not used by the kernel.

We can add a new vm_map_public.h header to separate the kernel-specific from the rest, if that separation is important.