sysfs interface to transparent hugepages

sysfs interface to transparent hugepages

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sysfs interface to transparent hugepages

[Andrea Arcangeli]

On Mon, Mar 21, 2011 at 03:00:31AM +0000, Ben Hutchings wrote:
> 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!

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.

> 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'.

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.

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

I don't get the question sorry.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: sysfs interface to transparent hugepages

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Mon, 2011-03-21 at 13:42 +0100, Andrea Arcangeli wrote:
> On Mon, Mar 21, 2011 at 03:00:31AM +0000, Ben Hutchings wrote:
[...]
> > 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'.
> 
> As said I like that format and I've been consistent in using it.

But not consistent with anything else in sysfs.

> If you write a parser for that format in userland it's probably easier to
> be consistent.

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'?

> 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.

I can do that, yes.

> 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.

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

> > And really, why add boolean flags for a tristate at all?
> 
> I don't get the question sorry.

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sysfs interface to transparent hugepages

[Andrea Arcangeli]

On Mon, Mar 21, 2011 at 01:13:03PM +0000, Ben Hutchings wrote:
> 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.

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: sysfs interface to transparent hugepages

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1157 bytes --]

On Mon, 2011-03-21 at 15:08 +0100, Andrea Arcangeli wrote:
> On Mon, Mar 21, 2011 at 01:13:03PM +0000, Ben Hutchings wrote:
> > 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.
> 
> 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).
[...]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]