Page MenuHomeFreeBSD

Pretty-print the kevent structures logged via KTRACE.
ClosedPublic

Authored by jhb on Sep 22 2017, 10:46 PM.

Details

Summary

This is a series of three commits (and I'll probably split them up
for committing) whose log messages are below, but it might be easier
to view the entire idea to get a sense of if it is the right direction
or not. A couple of issues / questions I have:

  1. Is KTR_STRUCT_ARRAY a better approach than having the kevent() just do a bunch of separate KTR_STRUCT records for each event? Note that if we do the latter we don't currently have a way to distinguish "in" events from "out" events, though perhaps we could use separate structure names like "kevent-in" and "kevent-out". Another use case in the future might be to log the 'struct pollfd' array passed to poll() for example.
  1. Does this need to be more explicit about logging "in" vs "out" structures? In the existing GENIO records, the in/out direction is used to indicate which set of kevent structures are being logged, but that is now only communicated implicitly as documented in the first commit log below. We could add another 'flag' field in the KTR_STRUCT_ARRAY header that could include "in" vs "out" perhaps?

Add a new KTR_STRUCT_ARRAY which dumps an array of structures.

The structure name in the record payload is preceded by a size_t
containing the size of the individual structures. Use this to
replace the previous code that dumped the kevent arrays dumped for
kevent(). kdump is now able to decode the kevent structures rather
than dumping their contents via a hexdump.

One change from before is that the 'changes' and 'events' arrays are
not marked with separate 'read' and 'write' annotations in kdump output.
Instead, the first array is the 'changes' array, and the second array
(only present if kevent doesn't fail with an error) is the 'events'
array. For kevent(), empty arrays are denoted by an entry with an array
containing zero entries rather than no record.

Move kevent decoding tables from truss to libsysdecode.

This adds three new functions to decode members of struct kevent:
sysdecode_kevent_filter, sysdecode_kevent_flags, and
sysdecode_kevent_fflags.

Use the new sysdecode_kevent_* functions when printing kevent structures.

Test Plan
  • ktrace / kdump of a test program using several kevent types

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Sep 22 2017, 10:46 PM
kib added inline comments.Sep 23 2017, 12:47 PM
sys/kern/kern_event.c
956 ↗(On Diff #33345)

There is one issue there, note the kevent_size argument. The kern_kevent_generic() is called for native and for native compat11 kevents. I think it would be useful to utilize the kstructarray name and e.g. use "event-11" for compat case.

Also, it might be possible now to dump kevent32 and kevent32-11 from the compat32 layer.

jhb added inline comments.Nov 21 2017, 7:45 PM
sys/kern/kern_event.c
956 ↗(On Diff #33345)

I can actually switch on the structure size in kdump to handle these without requiring a separate name. I do agree that we could add dumping of kevent32 structures, but would probably do that as a separate change.

kib added inline comments.Nov 21 2017, 8:19 PM
sys/kern/kern_event.c
956 ↗(On Diff #33345)

Relying on size is messy, it assumes that there is no collisions and never will be. It is mostly fine when we extend a single structure like lwpinfo, but there we have at least four different definitions of kevent{native,32}{12,11} and might be more in future. I would be not suprised if something goes wrong and some sizes appear same.

jhb updated this revision to Diff 35562.Nov 21 2017, 9:02 PM
  • Rebase.
  • Handle freebsd11 kevent structures.
kib added inline comments.Nov 22 2017, 10:52 AM
usr.bin/kdump/kdump.c
2150 ↗(On Diff #35562)

I do not understand this. Where is performed the convertion from kevent11 to kevent to handle the different struct layouts ? Unless I am mistaken, this memcpy lays the old structure' bits over the new one.

jhb added inline comments.Nov 22 2017, 8:29 PM
usr.bin/kdump/kdump.c
2150 ↗(On Diff #35562)

Yeah, I realized this last night and just haven't fixed it yet.

jhb updated this revision to Diff 35625.Nov 23 2017, 12:05 AM
  • Use an alternate name for kevent_freebsd11 structure.
  • Rename _WANT_KEVENT11 to _WANT_FREEBSD11_KEVENT to match stat11 macro.
  • Decode compat11.kevent() including kevent11 structures.
  • Properly convert kevent11 structures to native before displaying.
  • Constify the 'data' argument to ktrstruct*().
  • Save eventlist pointer since the copyout routine changes it.
  • Dump kevent structures for freebsd32 kevent system calls via KTRACE.
kib accepted this revision.Nov 23 2017, 10:40 AM

This looks fine.

I wonder if kernel could just dump the native struct kevent array before/after the ABI conversions to avoid one more conversion in libsysdecode. But since you already coded it this way, let it stay as is: it saves us from the issue of piece-wise processing of the events.

lib/libsysdecode/flags.c
589 ↗(On Diff #35625)

Why do you prefer to have fputs/fprintf instead of adding one more %s to the format and eliminate the fputs call ?

lib/libsysdecode/sysdecode_kevent.3
42 ↗(On Diff #35625)

Perhaps .In stdio.h is needed ?

sys/sys/event.h
103 ↗(On Diff #35625)

Perhaps this should be guarded by defined(_WANT_FREEBSD11_KEVENT) && defined(__WANT_KEVENT32))

This revision is now accepted and ready to land.Nov 23 2017, 10:40 AM
jhb added a comment.Nov 23 2017, 4:27 PM

I also considered always dumping the native format (and in that case one could perhaps use a bunch of KTRSTRUCT traces). You would always dump all structures in that case (so the genio rate limit in the existing traces wouldn't be honored easily), and you would not have a way to distinguish the 'in' vs 'out' events aside from looking at the args passed to kevent and the return value in the kdump output. I suppose I could skip tracing arrays of zero objects even in the current approach if I depend on the user to look at the kevent syscall args to determine if events are "in" vs "out". Note that for truss I will always have to deal with the alternate formats anyway.

lib/libsysdecode/sysdecode_kevent.3
42 ↗(On Diff #35625)

Hmm, it probably is for sysdecode_mask.3 as well and a few other functions which use FILE.

sys/sys/event.h
103 ↗(On Diff #35625)

It effectively is because it is nested under the earlier #if.

kib added a comment.Nov 23 2017, 6:03 PM

Indeed, truss should be able to decode all variants of the structure, so libsysdecode must support it anyway.

sys/sys/event.h
103 ↗(On Diff #35625)

I see.

jhb added a comment.Nov 23 2017, 7:01 PM

Well, the libsysdecode bits always work on native formats (and even then they only work on individual fields). Both truss and kdump convert the alternate formats to the native one before decoding any fields. However, the conversion code in truss requires that all the structure variants are available to userland via _WANT_*. However, if you think it would be cleaner/better to dump individual kevent structures always in native format for ktrace/kdump that is relatively easy to handle. The pros vs cons for that approach is 1) you don't get a genio-style limit on the number of structures logged, and 2) you have to look at the kevent syscall args to infer which structure are "in" vs "out". However, it's not a clear cut argument either way IMO. I mostly stick with the 'ktrstructarray' approach because it is most similar to what is there now, but it does result in extract complexity in kdump. (Also, it might be somewhat odd for a user to pick out what happened if kevent tries to write out records and gets an EFAULT because the records would be logged and then an EFAULT error reported in the ktrace output.)

kib added a comment.Nov 23 2017, 7:13 PM

I think that the current patch should be committed as is. It is a significant improvement over the previous state to have the compat32 kevents dumped, and for all variants of them decoded.

If I decide later that I want to tweak this code I will do.

jhb updated this revision to Diff 35744.Nov 24 2017, 10:43 PM
  • Add missing stdio.h includes.
  • Consolidate an fputs and fprintf into a single fprintf.
This revision now requires review to proceed.Nov 24 2017, 10:43 PM
jhb marked 3 inline comments as done.Nov 24 2017, 10:44 PM
This revision was automatically updated to reflect the committed changes.