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

Change unit conversion from KiB to KB (and all equivalents) #241

Closed
GuillaumeGomez opened this issue Jan 11, 2020 · 13 comments
Closed

Change unit conversion from KiB to KB (and all equivalents) #241

GuillaumeGomez opened this issue Jan 11, 2020 · 13 comments

Comments

@GuillaumeGomez
Copy link
Owner

This is the follow-up of #240 and #239.

@vyamkovyi
Copy link
Contributor

Now this is very, very confusing.

@GuillaumeGomez
Copy link
Owner Author

I don't get your comment, what do you mean?

@vyamkovyi
Copy link
Contributor

On Windows, this is actually KB already, and on Linux it is still KiB, so calculations like system_info.get_available_memory() / 1000000 are broken until this issue gets fixed. I'll try to make a PR today.

@GuillaumeGomez
Copy link
Owner Author

From the proc man, it says:

       /proc/meminfo
              This  file  reports statistics about memory usage on the system.  It is used by free(1)
              to report the amount of free and used memory (both physical and swap) on the system  as
              well  as  the shared memory and buffers used by the kernel.  Each line of the file con‐
              sists of a parameter name, followed by a colon, the value of the parameter, and an  op‐
              tion  unit  of  measurement (e.g., "kB").  The list below describes the parameter names
              and the format specifier required to read the field value.  Except as noted below,  all
              of  the fields have been present since at least Linux 2.6.0.  Some fields are displayed
              only if the kernel was configured with various options; those dependencies are noted in
              the list.

So it seems like the unit is right, or did I miss something?

@vyamkovyi
Copy link
Contributor

vyamkovyi commented Feb 9, 2021

Whatever it says, take a look at the kernel source. man is lying. You can divide free -b result by 1024 and get the result from /proc/meminfo to verify that, as another way.

@GuillaumeGomez
Copy link
Owner Author

GuillaumeGomez commented Feb 9, 2021

Wow indeed:

static void show_val_kb(struct seq_file *m, const char *s, unsigned long num)
{
	seq_put_decimal_ull_width(m, s, num << (PAGE_SHIFT - 10), 8);
	seq_write(m, " kB\n", 4);
}

And PAGE_SHIFT is defined as follow:

#define PAGE_SHIFT	13

Which means that all values get moved 3 bits to the left, meaning they're multipled by 8 if I'm not wrong. And yet, they still print kB and not kiB. This is really weird...

@GuillaumeGomez
Copy link
Owner Author

GuillaumeGomez commented Feb 9, 2021

After taking a look at htop, they take the values as is: https://github.com/htop-dev/htop/blob/7ba3396a4c3269f6d26b52f4f4fac72a8b49f25d/linux/LinuxProcessList.c#L1589

I also discovered that they were displaying kiB and not kB (which explains why the value seemed correct). Meaning that the current is indeed invalid. I'd be very glad if you opened a PR as you proposed to fix this issue! :)

@vyamkovyi
Copy link
Contributor

vyamkovyi commented Feb 9, 2021

Just for future reader, this actually can be explained. I think meminfo-related code could be written before 1998 (Linux was started in 1991), which was the year when the IEC finally clarified the confusion. Anyway, I'd like to remind readers that /proc filesystem is obsolete and future applications should look into using /sys on Linux.

Perhaps offtopic, but I'd also like to point out that Linux kernel config documentations still have "it is recommended to say Y" for options that are absolutely obsolete (and vice-versa), which suggests that the kernel simply does not accept patches with documentation updates.

@GuillaumeGomez
Copy link
Owner Author

That's very interesting. I wasn't aware that /sys was supposed to provide process information too. Would you be interesting into making the migration?

@GuillaumeGomez
Copy link
Owner Author

Actually, from what I was able to find around, the processes information are not available in the sysfs. Very weird (and a bit disappointing)...

@vyamkovyi
Copy link
Contributor

vyamkovyi commented Feb 9, 2021

You are not exactly right, the information we are talking about can be found at /sys/devices/system/node/node0/meminfo (note that nodeX which actually makes it a little harder to replace /proc/meminfo use properly). Interestingly enough, it is lying about units as well.

@GuillaumeGomez
Copy link
Owner Author

What the hell. 🤣

vyamkovyi pushed a commit to vyamkovyi/sysinfo that referenced this issue Feb 9, 2021
vyamkovyi pushed a commit to vyamkovyi/sysinfo that referenced this issue Feb 9, 2021
@GuillaumeGomez
Copy link
Owner Author

I wonder: is it the same problem for the processes' memory too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants