Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

macOS: buildTopology in kernel now fills output buffer directly #440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

macOS: buildTopology in kernel now fills output buffer directly #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 3 additions & 23 deletions src/MacMSRDriver/PcmMsr/PcmMsr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,32 +168,12 @@ IOReturn PcmMsrDriverClassName::writeMSR(pcm_msr_data_t* idata){

IOReturn PcmMsrDriverClassName::buildTopology(topologyEntry* odata, uint32_t input_num_cores)
{
size_t topologyBufferSize;

// TODO figure out when input_num_cores is used rather than num_cores
if (os_mul_overflow(sizeof(topologyEntry), (size_t) num_cores, &topologyBufferSize))
{
return kIOReturnBadArgument;
}

topologyEntry *topologies =
(topologyEntry *)IOMallocAligned(topologyBufferSize, 32);

if (topologies == nullptr)
{
return kIOReturnNoMemory;
if (odata == nullptr) {
return kIOReturnBadArgument;
}

mp_rendezvous_no_intrs(cpuGetTopoData, (void*)topologies);

for(uint32_t i = 0; i < num_cores && i < input_num_cores; i++)
{
odata[i].core_id = topologies[i].core_id;
odata[i].os_id = topologies[i].os_id;
odata[i].socket = topologies[i].socket;
}
mp_rendezvous_no_intrs(cpuGetTopoData, (void*)odata);

IOFreeAligned(topologies, topologyBufferSize);
return kIOReturnSuccess;
}

Expand Down
7 changes: 5 additions & 2 deletions src/MacMSRDriver/PcmMsr/UserKernelShared.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can look into this. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, I'll make the change.

// needs to be kept in sync with the one in cpucounters.h (at least,
// the first 3 fields). Ideally we would just include that, but
// cpucounters.h has dependencies on the platform SDK and cannot be
// compiled in the kernel on macOS today.
typedef struct
{
int32_t os_id;
Expand All @@ -30,7 +34,6 @@ typedef struct
int32_t socket;
int32_t native_cpu_model;
int32_t core_type; // This is an enum in the userland structure.
int32_t padding;
} topologyEntry;

enum {
Expand Down