Attempt to "Fix" OpenBSD Build #1277

time-killer-games · 2023-08-08T10:51:13Z

I will need someone to do additional research and make sure this is correct as I'd like to not be responsible for further breakage to what is already borked.

Now unused/free RAM matches OpenBSD top(1) and the following two code examples:

 int mib[2];
 mib[0] = CTL_VM;
 mib[1] = VM_UVMEXP;
 struct uvmexp buf;
 std::size_t sz = sizeof(buf);
 if (!sysctl(mib, 2, &buf, &sz, nullptr, 0))
   freeram = buf.free * sysconf(_SC_PAGESIZE);

freeram = (sysconf(_SC_AVPHYS_PAGES) * sysconf(_SC_PAGESIZE));

On the left is my example program. On the right is htop from this pr. Note my example program rounds up to the nearest 0.01 and htop does not. Although it's probably wrong I removed the code logic for getting the cached memory completely.

openbsd/OpenBSDMachine.c

BenBE · 2023-08-08T11:06:44Z

openbsd/OpenBSDMachine.c

@@ -92,11 +92,12 @@ Machine* Machine_new(UsersTable* usersTable, uid_t userId) {
   char errbuf[_POSIX2_LINE_MAX];

   OpenBSDMachine* this = xCalloc(1, sizeof(OpenBSDMachine));
-   Machine* super = &this->super;
+   Machine* super = (Machine*)&this->super;


What's the error message for this? this->super should be of type Machine in openbsd/OpenBSDMachine.h.

openbsd/OpenBSDMachine.c

BenBE · 2023-08-08T11:19:04Z

openbsd/OpenBSDMachine.c

@@ -129,7 +130,7 @@ void Machine_delete(Machine* super) {
 }

 static void OpenBSDMachine_scanMemoryInfo(OpenBSDMachine* this) {
-   Machine* host = &this->super;
+   Machine* super = (Machine*)&this->super;


There should be no cast necessary. Please provide build message.

iirc it was saying there was a cast needed, which is odd. I didn't think that was necessary in C.

I'll revert the casts and show you the errors I'm getting.

Ah, i see now; they were warnings, not errors. In that case I'll revert that.

Can you post the warnings you are getting? Just to check if there's anything funny going on.

Sorry, I was asleep. Getting that information now.

It's the same warning that can be found in other sources I did not modify, such as this one:

OpenBSDProcessList.c:41:11: warning: incompatible pointer types returning 'OpenBSDProcessList *' (aka 'struct OpenBSDProcessList_ *') from a function with result type 'ProcessList *' (aka 'struct ProcessList_ *') [-Wincompatible-pointer-types]

There seems to be 3 other instances of this warning:

OpenBSDMachine.c:117:11: warning: incompatible pointer types returning 'OpenBSDMachine *' (aka 'struct OpenBSDMachine_ *') from a function with result type 'Machine *' (aka 'struct Machine_ *') [-Wincompatible-pointer-types]

OpenBSDMachine.c:121:20: warning: incompatible pointer types initializing 'OpenBSDMachine *' (aka 'struct OpenBSDMachine_ *') with an expression of type 'Machine *' (aka 'struct Machine_ *') [-Wincompatible-pointer-types]

OpenBSDMachine.c:265:20: warning: incompatible pointer types initializing 'OpenBSDMachine *' (aka 'struct OpenBSDMachine_ *') with an expression of type 'Machine *' (aka 'struct Machine_ *') [-Wincompatible-pointer-types]

BenBE · 2023-08-08T11:20:20Z

openbsd/OpenBSDMachine.c

-
-   // Taken from OpenBSD systat/iostat.c, top/machine.c and uvm_sysctl(9)
-   const int bcache_mib[] = { CTL_VFS, VFS_GENERIC, VFS_BCACHESTAT };
-   struct bcachestats bcstats;
-   size_t size_bcstats = sizeof(bcstats);
-
-   if (sysctl(bcache_mib, 3, &bcstats, &size_bcstats, NULL, 0) < 0) {
-      CRT_fatalError("cannot get vfs.bcachestat");
-   }
-
-   super->cachedMem = bcstats.numbufpages * this->pageSizeKB;


Why is this part of the code removed?

I'm not sure what purpose the cached memory served, as I wasn't seeing that printed to the terminal as it's own stat in specific, although it does effect the amount of used ram reported, which is part of what made it not match up with top(1).

There'S a later place in the code that calculates the used memory shown in the Memory meter. Also the cached memory (i.e. file system cache) is not directly printed there, but shown as blue (file buffers/shared memory) or brown (volatile file system cache).

htop tries to track some more resources separately, thus things might seem unintuitive at first.

So do you consider the used/free memory not matching top(1) to be intentional? If not, we will need to fix that, otherwise, this code can be put back and not much else needs changing at this point.

The intended display is used+compressed+buffers+shared for the used value in the memory bar; the cached value is ignored intentionally, as that's memory that can usually be reclaimed by the kernel if needed. And although cached resides in RAM, it's not usually considered as "used" for htop due to this volatile nature.

used: Memory used by programs, libraries, data
compressed: Memory compressed by te kernel to avoid swapping (e.g. zram)
buffers: File buffers & dirty pages for I/O
shared: Shared memory segments, memory used e.g. by tmpfs
cached: volatile file system cache (e.g. recently read data to optimize disk access)

Example from Linux:

Although there's practically no memory unused by Linux, the meter declares the more reasonable value of ~20GiB of memory that cannot easily be dropped, but would need to be placed somewhere else if some new program wanted to use too much memory.

I do want to make this a different pull request so this doesn't get conflated, but I think it would be good if NetBSD used code which was more in line with what OpenBSD is doing with the sysctl(), VM_UVMEXP, and swapctl(). NetBSD could be using near identical code for these things to reduce needed maintenance, but instead it is using sysctl() and the VM_UVMEXP2 struct, which is like the VM_UVMEXP struct but also covers what OpenBSD is doing to get swap related info with swapctl(). We'll cross that bridge when we get there, perhaps I'll link to this comment from my new pull request once this pull request is done.

@BenBE I almost forgot about this part; did you want me to add back the code logic I took out here? Again, doing this will make it not print the correct amount of ram and it will no longer align with what OpenBSD's stock top cli prints for ram.

Given that this change drops initializing the super->cachedMem member of the Machine structure, I'm inclined to have this block of code stay or be updated and follow the semantics used within htop.

The memory metrics in htop mostly fall into 4 categories (simplified):

used: RAM used by a program for working memory, code or other resources

shared: RAM shared between processes, like shmem or mmap'd files

buffers: RAM allocated for write-back buffers of the filesystem

caches: RAM used as filesystem (read) cache that could be dropped at any moment

Of those categories 1 and 2 are subject to potential swapping, while 4 is (mostly) never swapped. Category 3 is basically what you'd first write to disk, if you were to need some more free RAM.

This is much simplified, and differs slightly from what top displays regarding the handling of shared memory to accommodate for things like tmpfs/ramfs on Linux.

AFAICS, the code that was removed should have worked in this regard and if there's a need for that code to change I'd recommend to split out that change as a separate PR to separate build fixes from behavioral changes.

BenBE · 2023-08-08T11:23:52Z

Please keep behavior changes separate from build fixes.

Can go in with one single PR, but should be distinct commits. If necessary you may also group the build fixes as necessary. TIA.

time-killer-games · 2023-08-08T11:36:50Z

i clicked commit on that by mistake i meant to revert them all at once.

time-killer-games · 2023-08-08T11:40:50Z

openbsd/OpenBSDMachine.c

@@ -273,7 +262,7 @@ static void OpenBSDMachine_scanCPUTime(OpenBSDMachine* this) {
 }

 void Machine_scan(Machine* super) {
-   OpenBSDMachine* this = (OpenBSDMachine*) super;
+   OpenBSDMachine* this = super;


I didn't know this cast was here initially. Let me know if you want me to add it back. Though removing it didn't mess up the build. I might redo this pr from scratch if i mess up too much.

The rule usually is casting from generic to specific (platform) type; no casting from platform type to generic (use ->super instead).

Also: Feel free to rebase/squash the commits to your hearts content, until they look clean.

After I'm done making all my changes locally to how we want the final result to look, i will create a new pr, close this one, and break it up into a couple commits, ones that have code changes related to one another in how it is broken up, without the excess in extra commits. Is that ok?

In the meantime, feel free to let me know how you want the commits broken up here.

Please instead of opening a new PR, just split the changes into commits as appropriate and push them (--force-with-lease) on the current branch.

Regarding splitting the commits:

One commit (or more, if there are vastly distinct build issues) for compile issues

One commit fixing the memory detection (see my other comment regarding that)

Apart from that, providing descriptive commit messages and following the information outlined in the style guide is much appreciated.

openbsd/OpenBSDMachine.c

Cherry-picked build-relevant part of #1277

natoscott · 2023-08-30T06:02:17Z

@time-killer-games I've merged the build-fix part of this PR, however lots of other changes have also been made in the meantime. Could you see if we have a clean OpenBSD build now please? Any issues, please let me know - thanks!

time-killer-games · 2023-09-20T01:13:48Z

@time-killer-games I've merged the build-fix part of this PR, however lots of other changes have also been made in the meantime. Could you see if we have a clean OpenBSD build now please? Any issues, please let me know - thanks!

Without going too far into details my probation officer has taken all my devices except my phone. I don't know when she will let me have my stuff again, but hopefully soon so i can do this for you. I apologize for the inconvenience.

BenBE · 2023-12-31T20:41:10Z

What's the status of this?

BenBE · 2024-01-02T16:54:40Z

@fraggerfox What about cross-checking this PR for #1360? They seem to address similar issues.

time-killer-games · 2024-01-05T02:57:34Z

@time-killer-games I've merged the build-fix part of this PR, however lots of other changes have also been made in the meantime. Could you see if we have a clean OpenBSD build now please? Any issues, please let me know - thanks!

I am going to attempt to resolve conflicts and we will go from there.

Edit: There was only one small conflict, so it wasn't as bad as I thought.

That said, I am going to test this pr again just to be sure nothing broke.

time-killer-games · 2024-01-05T03:47:23Z

Alright, tried building in the current state, and got these errors:

openbsd/OpenBSDProcessTable.c:48:55: error: use of undeclared identifier 'super'
OpenBSDProcessTable* this = (OpenBSDProcessTable*) super;
^
openbsd/OpenBSDProcessTable.c:47:34: warning: unused parameter 'cast' [-Wunused-parameter]
void ProcessTable_delete(Object* cast) {
^
openbsd/OpenBSDProcessTable.c:133:32: error: no member named 'host' in 'struct ProcessTable_'
Machine* host = this->super.host;
~~~~~~~~~~~ ^
2 warnings and 2 errors generated.
*** Error 1 in . (Makefile:1549 'openbsd/OpenBSDProcessTable.o')
*** Error 2 in /home/openbsd/htop (Makefile:1095 'all')

I have no clue what to do now.

BenBE · 2024-01-05T14:15:32Z

The first error is with the cast needing cast instead of super. This is given that you get Object * (any parent class) instead of the direct parent (which would be named super).

This second one is inheritance-related as that field in question moved one more parent up in the hierarchy (this->super.super.host, unchecked). There will likely be a similar check in #1360. Also compare with other platforms like in e.g. linux/LinuxProcessTable.c and their handling of this kind of access.

time-killer-games · 2024-01-05T19:09:20Z

lol the needing of cast instead of super was rather obvious. I think it didn't occur to me at first because I never bothered looking up where that error was located in the actual code.

Fixing these things now. Will report back if everything worked.

BenBE · 2024-01-05T19:20:31Z

Please do not merge, but rather rebase your changes onto the latest commits from main.

time-killer-games · 2024-01-05T19:22:27Z

I think, unless I'm sadly mistaken, we're all set! :)

time-killer-games · 2024-01-05T19:23:41Z

Please do not merge, but rather rebase your changes onto the latest commits from main.

Taking care of it now.

time-killer-games · 2024-01-05T19:41:36Z

@BenBE is it ok if i open a new pull request and close this one, to reduce the number of commits down to one commit? I ask because I am having a bit of trouble rebasing, I'm not very good with git and I'm concerned I might mess this up.

If so, what should be the title and description of the commit and name of the branch?

BenBE · 2024-01-05T22:05:52Z

Preferably things should be resolved in this PR as it minimizes clutter in the issue tracker. If you really need to open a new one you should mark the new one to superseed this one, cross-link both issues accordingly so people can follow up in the new one.

I did a rebase+squash of your changes (as present on this PR). Please recheck. HTH.

In case you want to get a bit more familiar with how git works, there's a nice game called Oh My Git that explains many of the basics and some advanced topics.

openbsd/OpenBSDMachine.c

time-killer-games · 2024-01-06T00:33:39Z

I think after you go over my two recent review comments with your thoughts, and we address those things, we should be all set. @BenBE I tested and verified it still works as intended imo.

Comparing htop with the output from my own cli ween / sysinfo from:

https://github.com/time-killer-games/ween

Co-authored-by: Benny Baumann <BenBE@geshi.org>

time-killer-games · 2024-01-06T08:03:10Z

@BenBE I've rebased well over a dozen times and every time i do it, it ruins all the code in the pull request and edits the code drastically in ways I'm not telling it to. Either we do this pr from scratch, or i give up. I ruined my entire night trying to figure out git, and never had a more terrible experience.

BenBE · 2024-01-06T14:30:20Z

Superseeded by #1361 …

BenBE reviewed Aug 8, 2023

View reviewed changes

BenBE added bug 🐛 Something isn't working build system 🔧 Affects the build system rather then the user experience BSD 🐡 Issues related to *BSD labels Aug 8, 2023

BenBE added this to the 3.3.0 milestone Aug 8, 2023

time-killer-games commented Aug 8, 2023

View reviewed changes

BenBE added the OpenBSD 🐡 OpenBSD related issues label Aug 8, 2023

BenBE reviewed Aug 9, 2023

View reviewed changes

openbsd/OpenBSDMachine.c Show resolved Hide resolved

natoscott added a commit that referenced this pull request Aug 30, 2023

Fix some OpenBSD build fallout from struct Machine changeset

f4e4f78

Cherry-picked build-relevant part of #1277

BenBE force-pushed the main-1 branch from cf59529 to a0f7854 Compare January 5, 2024 21:51

time-killer-games commented Jan 6, 2024

View reviewed changes

openbsd/OpenBSDMachine.c Outdated Show resolved Hide resolved

time-killer-games force-pushed the main-1 branch 3 times, most recently from 64f03bf to 4cbdf13 Compare January 6, 2024 05:35

time-killer-games force-pushed the main-1 branch 8 times, most recently from e447f3f to 1514004 Compare January 6, 2024 07:09

Attempt to "Fix" OpenBSD Build

f2a8005

Co-authored-by: Benny Baumann <BenBE@geshi.org>

time-killer-games force-pushed the main-1 branch from 15d30f6 to f2a8005 Compare January 6, 2024 07:47

time-killer-games closed this Jan 6, 2024

time-killer-games deleted the main-1 branch January 6, 2024 08:03

BenBE mentioned this pull request Jan 6, 2024

Attempt to "Fix" OpenBSD Build (Followup) #1361

Merged

Attempt to "Fix" OpenBSD Build #1277

Attempt to "Fix" OpenBSD Build #1277

Conversation

time-killer-games commented Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

time-killer-games Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE commented Aug 8, 2023

time-killer-games commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natoscott commented Aug 30, 2023

time-killer-games commented Sep 20, 2023 • edited Loading

BenBE commented Dec 31, 2023

BenBE commented Jan 2, 2024

time-killer-games commented Jan 5, 2024 • edited Loading

time-killer-games commented Jan 5, 2024 • edited Loading

BenBE commented Jan 5, 2024

time-killer-games commented Jan 5, 2024

BenBE commented Jan 5, 2024

time-killer-games commented Jan 5, 2024

time-killer-games commented Jan 5, 2024

time-killer-games commented Jan 5, 2024

BenBE commented Jan 5, 2024

time-killer-games commented Jan 6, 2024 • edited Loading

time-killer-games commented Jan 6, 2024 • edited Loading

BenBE commented Jan 6, 2024

time-killer-games commented Aug 8, 2023 •

edited

Loading

time-killer-games Jan 6, 2024 •

edited

Loading

time-killer-games commented Sep 20, 2023 •

edited

Loading

time-killer-games commented Jan 5, 2024 •

edited

Loading

time-killer-games commented Jan 5, 2024 •

edited

Loading

time-killer-games commented Jan 6, 2024 •

edited

Loading

time-killer-games commented Jan 6, 2024 •

edited

Loading