pmc: Provide full path to modules from kernel linker
ClosedPublic
Actions

Authored by jrtc27 on May 10 2023, 5:37 PM.

Details

Reviewers

jhb
jkoshy
mhorne
gnn

Commits

rG53d0b9e438bc: pmc: Provide full path to modules from kernel linker

Summary

This unifies the user object and kernel module paths in libpmcstat,
allows modules loaded from non-standard locations (e.g. from a user's
home directory when testing) to be found and, since buffer is what all
the warnings here use (they were never updated when buffer_modules were
added to pick based on where the file was found) has the side-effect of
ensuring the messages are correct.

This includes obsoleting the now-superfluous -k option in pmcstat.

This change breaks the hwpmc ABI and will be followed by a bump to the
pmc major version.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 51787
Build 48678: arc lint + arc unit

Event Timeline

jrtc27 created this revision.May 10 2023, 5:37 PM

Herald added a reviewer: gnn. · View Herald TranscriptMay 10 2023, 5:37 PM

Herald added a subscriber: imp. · View Herald Transcript

jrtc27 requested review of this revision.May 10 2023, 5:37 PM

Harbormaster completed remote builds in B51424: Diff 121815.May 10 2023, 5:37 PM

jrtc27 added a child revision: D40050: pmc: Bump major version for just-committed breaking changes.May 10 2023, 5:41 PM

The resulting cleanup is excellent.

sys/kern/kern_linker.c
2122	Just looking at the usage of this field, it could become an anonymous union of: union { char pathname; vnode vp; };

This revision is now accepted and ready to land.May 11 2023, 6:18 PM

In D40048#912416, @mhorne wrote:

The resulting cleanup is excellent.

Thank @jhb for the suggestion :)

sys/kern/kern_linker.c
2122	Doesn't need to be anonymous, really, but yeah, a union would be nicer than this slightly disgusting kernel-internal interface

jkoshy accepted this revision.May 12 2023, 6:07 AM

jkoshy added inline comments.

usr.sbin/pmcstat/pmcstat.c
625	Consider accepting this option for an additional release cycle, while issuing a warning that it no longer has an effect.

I'm inclined to agree with @jkoshy with emitting a warning for -k for stable/14, but the error from usage for an unknown option might be just as functional in practice.

I'm happy to do that if consensus is that's better, I just felt that it's better to give a hard error for something that's been removed as an option rather than warning that the option that used to do something now doesn't do anything but continuing onwards. I guess it depends whether people are doing weird things with -k; if they're only using it to work around the lack of full paths then ignoring the option will do the right thing, but if they're using it to map onto some other directory that doesn't match the full path then it'll do the wrong thing. Probably we're in the former category though, especially given how little pmcstat actually gets used...

In D40048#915407, @jrtc27 wrote:

I'm happy to do that if consensus is that's better, I just felt that it's better to give a hard error for something that's been removed as an option rather than warning that the option that used to do something now doesn't do anything but continuing onwards. I guess it depends whether people are doing weird things with -k; if they're only using it to work around the lack of full paths then ignoring the option will do the right thing, but if they're using it to map onto some other directory that doesn't match the full path then it'll do the wrong thing. Probably we're in the former category though, especially given how little pmcstat actually gets used...

For me it's more of a small preference, but not a strong one either way.