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.

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

NetBSD: Rework CPU counting. #718

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Conversation

fraggerfox
Copy link
Contributor

This is the continuation of the work done in #656

Refactors the code in NetBSD to comply with the changes.

netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/Platform.c Outdated Show resolved Hide resolved
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

What's wrong with writing it like this?

netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
@fraggerfox fraggerfox requested a review from BenBE August 3, 2021 15:57
@BenBE BenBE added enhancement Extension or improvement to existing feature NetBSD 🎏 NetBSD related issues labels Aug 3, 2021
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.h Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Show resolved Hide resolved
@fraggerfox
Copy link
Contributor Author

@BenBE I should probably visit the code base again once the merge is done, and refactor some of the variable names used, I liked the above suggestion to give it meaningful names in a self-documenting style.

@BenBE BenBE force-pushed the netbsd_cpu_count branch 3 times, most recently from bbc5614 to 305d52b Compare August 4, 2021 21:27
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

When tweaking some final stuff I noticed one last thing with the online field in the CPUData structure: The field is only ever written for the average (CPU 0), but never for any of the actual cores (1..super->activeCPUs). Please recheck if there is anything missing or drop a short note if I just missed some code that properly updates the online information.

@fraggerfox
Copy link
Contributor Author

fraggerfox commented Aug 5, 2021

When tweaking some final stuff I noticed one last thing with the online field in the CPUData structure: The field is only ever written for the average (CPU 0), but never for any of the actual cores (1..super->activeCPUs). Please recheck if there is anything missing or drop a short note if I just missed some code that properly updates the online information.

You are right, I have only done minimal implementation of CPU counting and I looked around in the documentation to understand how to do check on multiple CPUs and see if online / offline CPUs can be detected. Due to the lack of time and being unable to find the exact information I am looking for, I went for the base implementation of having to deal only with CPU 0. I will need a bit more time from my end to figure out how multi CPU stuff works in NetBSD.

I can do a follow up with @alarixnia and see if I can learn more about this after the merge.

@fraggerfox fraggerfox requested a review from BenBE August 5, 2021 05:12
@BenBE
Copy link
Member

BenBE commented Aug 5, 2021

Okay, then I'd suggest to skip the online field in those structures for now and add it in, when it gives practical information.

For now I'd propose to amend as follows and do the per-core online status in a follow-up PR:

$ git diff
diff --git a/netbsd/NetBSDProcessList.c b/netbsd/NetBSDProcessList.c
index c35946ae..a3530de7 100644
--- a/netbsd/NetBSDProcessList.c
+++ b/netbsd/NetBSDProcessList.c
@@ -93,7 +93,6 @@ static void NetBSDProcessList_updateCPUcount(ProcessList* super) {
       memset(dAvg, '\0', sizeof(CPUData));
       dAvg->totalTime = 1;
       dAvg->totalPeriod = 1;
-      dAvg->online = true;
 
       for (unsigned int i = 0; i < super->existingCPUs; i++) {
          CPUData* d = &opl->cpuData[i + 1];
@@ -480,5 +479,5 @@ bool ProcessList_isCPUonline(const ProcessList* super, unsigned int id) {
    assert(id < super->existingCPUs);
 
    const NetBSDProcessList* npl = (const NetBSDProcessList*) super;
-   return npl->cpuData[id + 1].online;
+   return true;
 }
diff --git a/netbsd/NetBSDProcessList.h b/netbsd/NetBSDProcessList.h
index 14b09312..1d1407d8 100644
--- a/netbsd/NetBSDProcessList.h
+++ b/netbsd/NetBSDProcessList.h
@@ -39,7 +39,6 @@ typedef struct CPUData_ {
    unsigned long long int idlePeriod;
 
    double frequency;
-   bool online;
 } CPUData;
 
 typedef struct NetBSDProcessList_ {

@fraggerfox
Copy link
Contributor Author

Okay, then I'd suggest to skip the online field in those structures for now and add it in, when it gives practical information.

For now I'd propose to amend as follows and do the per-core online status in a follow-up PR:

$ git diff
diff --git a/netbsd/NetBSDProcessList.c b/netbsd/NetBSDProcessList.c
index c35946ae..a3530de7 100644
--- a/netbsd/NetBSDProcessList.c
+++ b/netbsd/NetBSDProcessList.c
@@ -93,7 +93,6 @@ static void NetBSDProcessList_updateCPUcount(ProcessList* super) {
       memset(dAvg, '\0', sizeof(CPUData));
       dAvg->totalTime = 1;
       dAvg->totalPeriod = 1;
-      dAvg->online = true;
 
       for (unsigned int i = 0; i < super->existingCPUs; i++) {
          CPUData* d = &opl->cpuData[i + 1];
@@ -480,5 +479,5 @@ bool ProcessList_isCPUonline(const ProcessList* super, unsigned int id) {
    assert(id < super->existingCPUs);
 
    const NetBSDProcessList* npl = (const NetBSDProcessList*) super;
-   return npl->cpuData[id + 1].online;
+   return true;
 }
diff --git a/netbsd/NetBSDProcessList.h b/netbsd/NetBSDProcessList.h
index 14b09312..1d1407d8 100644
--- a/netbsd/NetBSDProcessList.h
+++ b/netbsd/NetBSDProcessList.h
@@ -39,7 +39,6 @@ typedef struct CPUData_ {
    unsigned long long int idlePeriod;
 
    double frequency;
-   bool online;
 } CPUData;
 
 typedef struct NetBSDProcessList_ {

Should be done in the latest commit, also put a TODO reminder for this in the code.

@BenBE BenBE added this to the 3.1.0 milestone Aug 5, 2021
@BenBE BenBE merged commit 2e3f34f into htop-dev:master Aug 5, 2021
@fraggerfox fraggerfox deleted the netbsd_cpu_count branch August 5, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature NetBSD 🎏 NetBSD related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants