macOS: buildTopology in kernel now fills output buffer directly #440
Conversation
| @@ -20,7 +20,11 @@ typedef struct { | |||
| char padding[115]; | |||
| } k_pcm_msr_data_t; | |||
|
|
|||
| // The topologyEntry struct that is used by PCM | |||
| // The topologyEntry struct that is used by PCM in the kernel. It | |||
There was a problem hiding this comment.
could you please try to move TopologyEntry out of cpucounters.h to a separate header that could be included into both UserKernelShared.h and cpucounters.h ?
There was a problem hiding this comment.
Yeah, I can look into this. Thanks.
There was a problem hiding this comment.
I spent some time on this today, and I see a couple of options - let me know if I missed something:
- Move TopologyEntry into it's own header file, which will work and is better than duplicating the structure
- Keep TopologyEntry in a header file that can be included that contains other types that are similar to it (needed in both kernel and user land), perhaps even an existing header in which user-land dependencies are removed from.
When trying the second, I ran into a lot of, e.g. iostream use in the header files, and, to start, I tried moving the calls in types.h to a new types.cpp file and using forward declarations but it ended up getting pretty messy and involving changes through a bunch of other headers (Some files which #include types.h need its includes)
I could do the first option, but it seems to me that some work such as running clang's "include-what-you-use" would help a great deal, and then it would be easier to have a header file for TopologyEntry and any other types that need to be included without pulling in the standard library. Put another way, once the include graph is a little cleaner, we would probably undo the first option if we did that now.
Can I propose to keep the duplicate TopologyEntry structure, and take it on as a TODO to start working on header file dependencies? For me it's a bit too big of a change to clean up all the headers at once on a new codebase, but I can definitely start going file by file, and keeping a duplicate TopologyEntry structure is not technically a regression to today.
Thank you!
There was a problem hiding this comment.
thank you for proposing the options. I agree with you that "include-what-you-use" is something we should do but it will take time... Thanks to you your work I realized that we have two topology structs and it is a significant issue that we need to address asap. Option 1 is easy to implement and I prefer to have it implemented in this patch rather than waiting for a later major change.
There was a problem hiding this comment.
Sounds great, I'll make the change.
Will do |
Previously, buildTopology would allocate a buffer, fill it, and copy it to the output buffer. The buffer passed from userland is of the same type that the kernel needs, and it is easier to fill it directly. Also added comment clarifying need to keep types in user/kernel land in sync.
nealsid
force-pushed
the
macos-toplogy-structure-mismatch
branch
from
f5026f8
to
b363f43
Compare
Previously, buildTopology would allocate a buffer, fill it, and copy it to the output buffer. The buffer passed from userland is of the same type that the kernel needs, and it is easier to fill it directly. Also added comment clarifying need to keep types in user/kernel land in sync.