sysfs interface to transparent hugepages

This kind of cute format:

       if (test_bit(enabled, &transparent_hugepage_flags)) {
               VM_BUG_ON(test_bit(req_madv, &transparent_hugepage_flags));
               return sprintf(buf, "[always] madvise never\n");
       } else if (test_bit(req_madv, &transparent_hugepage_flags))
               return sprintf(buf, "always [madvise] never\n");
       else
               return sprintf(buf, "always madvise [never]\n");

is probably nice for a kernel developer or experimental user poking around in sysfs. But sysfs is mostly meant for programs to read and write, and this format is unnecessarily complex for a program to parse.

Please use separate attributes for the current value and available values, like cpufreq does. I know there are other examples of the above format, but not everything already in sysfs is a *good* example!

This, on the other hand, is totally ridiculous:

       if (test_bit(flag, &transparent_hugepage_flags))
               return sprintf(buf, "[yes] no\n");
       else
               return sprintf(buf, "yes [no]\n");

Why show the possible values of a boolean? I can't even find any examples of 'yes' and 'no' rather than '1' and '0'.

And really, why add boolean flags for a tristate at all?

Ben.

Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Well I liked the io scheduler format the most as you may have guessed:

noop deadline [cfq]

so I used exactly that format... I didn't invent it. I found that the most intuitive and simpler so you deal with a single file, it's faster and more intuitive to use when you're on the shell and you twiddle with the values. You simply cannot get it wrong.

As said I like that format and I've been consistent in using it. If you write a parser for that format in userland it's probably easier to be consistent. Anyway this got into 2.6.38 only. For other kernels that shipped THP before 2.6.38 there is no /sys/kernel/mm/transparent_hugepage directory at all (it's renamed exactly to avoid any risk of sysfs ABI clashes). I doubt anybody wrote any parser for /sys/kernel/mm/transparent_hugepage so if this is a big deal I suggest you send patches to whatever you prefer. Or if you tell me exactly how you want it, I can try to implement it and if others agree I don't see a problem in altering it. But others may disagree. Clearly best would have been if you requested a change during 2.6.38-rc, everyone was aware of the format as everyone has been twiddling with these sysfs controls. Comments welcome.

I don't get the question sorry.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[...]

But not consistent with anything else in sysfs.

What if I already have some general functions like read_intr_attr(), read_bool_attr(), etc. Should I really have to write special functions for booleans in different parts of sysfs, depending on whether the author liked 0/1, false/true, disabled/enabled, no/yes, or 'yes [no]'/'[yes] no'?

I can do that, yes.

Sorry, I'm a distribution maintainer and I can't be everywhere.

You have tristates {never, madvise, always} for various THM features. Internally, these are represented as a pair of flags. They are exposed through sysfs as tristates, but then they are also exposed as flags.

Ben.

Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

They must be bitflags for performance and cacheline saving reasons in the kernel (1 bitflag not enough in kernel for a userland tristate). They're more intuitive as tristate in the same file for the user to set (some combination of these flags is forbidden so exposing the flags to the user doesn't sound good idea, also considering it's an internal representation which may change, keeping the two separated is best, especially if you want your current lib not to break).

There is no expectation however that you have to alter any of these settings even in server environment other than for debugging purposes:
with the exception of: 1) pages_to_scan, 2) scan_sleep_millisecs 3) alloc_sleep_millisecs inside the khugepaged dir, and those three are in a format that your current sysfs lib will mangle just fine.

If you've a lib that pretends to turn off THP as root, you may as well handle the cfq/deadline I/O scheduler switch in the same lib. Not really sure if your effort is worth it considering it will slightly complicate things in shell usage for debug purposes (I'd find more intuitive if also cpufreq governors were shown and selected like io schedulers).

But again I'm fully neutral on issues like these as long as the patches don't break anything I'm surely fine if others like your changes.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

[...]

I understand all that. However when I first looked at this I somehow thought that the tristate values were *also* exposed as flags, but I was mistaken. Sorry to bother you with this non-issue.

Ben.

Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.