Fix output of linprocfs stat entry

[chuck]

Summary

The Linux /proc/stat entry has grown over time

v2.5.41 <

user, nice, system, idle

v2.5.41

user, nice, system, idle, iowait, irq

v2.6.11

user, nice, system, idle, iowait, irq, softirq, steal

v2.6.24

user, nice, system, idle, iowait, irq, softirq, steal, guest

v2.6.32 >

user, nice, system, idle, iowait, irq, softirq, steal, guest, guest_nice

Some applications (e.g. nodejs) depend on the correct number of entries
and will abort otherwise.

Fix is to print the correct number of entries based on the value of
osrelease set either in sysctl or the jail settings. Change is similar
to approach used by illumos.

[trasz]

sys/compat/linprocfs/linprocfs.c

538 ↗

(On Diff #43924)

Can you actually do that? I mean, I suppose it works, otherwise you wouldn't submit it, but I'm still somewhat uneasy about that approach.

Perhaps just call sbuf_printf(3) twice per line instead, with the first one (four fields) common for all cases? This would also reduce code duplication (well, format strings duplication) a bit.

[emaste]

I like the idea, but I think the version comparison ought to be simplified and made more general. What do you think about parsing the version into a __FreeBSD_version style single integer in the sysctl handler and simplifying any comparisons of the value?

[emaste]

See linux_map_osrel

[chuck]

[v2] Fix output of linprocfs stat

Change sbuf_printf() approach as suggested by @trasz
Also fixed a typo with the per-CPU stats introduced with the patch

[chuck]

In D15858#336162, @emaste wrote:

See linux_map_osrel

This probably fits better with the existing code, and I'm inclined to move that direction. Is using linux_kernver() the way to access this value? If so, existing code appears to be comparing the returned value to values created via the LINUX_KERNVER() macro (in sys/compat/linux/linux_mib.h). But this macro creates a version number by bit shifting instead of multiplying. Any idea if linux_map_osrel() is wrong (i.e. should be using bit shifts) or if the macro is wrong (i.e. should be using multiplication)?

sys/compat/linprocfs/linprocfs.c

538 ↗

(On Diff #43924)

@trasz See if the updated diff is closer to what you were thinking.

[trasz]

sys/compat/linprocfs/linprocfs.c

538 ↗

(On Diff #43924)

That's it, thanks!

[emaste]

In D15858#336639, @chuck wrote:

In D15858#336162, @emaste wrote:

See linux_map_osrel

This probably fits better with the existing code, and I'm inclined to move that direction. Is using linux_kernver() the way to access this value?

Hrm, I hadn't yet looked in detail at this; checking now I see: sysctl compat.linux.osrelease ("Linux kernel OS release") invokes linux_set_osrelease, which calls linux_map_osrel, storing that result in (struct linux_prison *)lpr->pr_osrel as well as storing the string in lpr->pr_osrelease. linux_kernver fetches lpr->pr_osrel.

If so, existing code appears to be comparing the returned value to values created via the LINUX_KERNVER() macro (in sys/compat/linux/linux_mib.h).

Yeah, this definitely looks like a bug.

But this macro creates a version number by bit shifting instead of multiplying. Any idea if linux_map_osrel() is wrong (i.e. should be using bit shifts) or if the macro is wrong (i.e. should be using multiplication)?

I think it's our (FreeBSD's) choice. For FreeBSD the decimal shift (multiplication) method makes sense, as it matches what we store in ELF objects in our NT_ABI_TAG ELF note. Linux uses NT_GNU_ABI_TAG which consists of four words (OS descriptor, major, minor, subminor) so there's no precedent for us to follow. We could look at the history of the related files/functions/macros, but I'd be inclined to just go with the multiplication approach as it feels more natural for FreeBSD.

[chuck]

Added PR 229209 to track the osrelease issue and posted a proposed fix in https://reviews.freebsd.org/D15952

[chuck]

[v3] Fix output of linprocfs stat

Use linux_kernver() instead of roll a new "compare the kernel versions"
function as suggested by @emaste

[emaste]

sys/compat/linprocfs/linprocfs.c

478 ↗

(On Diff #44227)

Perhaps drop the commas from the comment, since the output format doesn't have them? I was a bit confused at first by zero_pad because of this.

[chuck]

sys/compat/linprocfs/linprocfs.c

478 ↗

(On Diff #44227)

Commas have been removed