| Message ID | a06810c216a45e5f6f1b9f49fbe2f332ca3c8972.1604261483.git.joe@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: Convert sysfs sprintf family to sysfs_emit | expand |
On Sun, Nov 01, 2020 at 12:12:51PM -0800, Joe Perches wrote: > @@ -4024,7 +4024,7 @@ int __init shmem_init(void) > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS) > static ssize_t shmem_enabled_show(struct kobject *kobj, > - struct kobj_attribute *attr, char *buf) > + struct kobj_attribute *attr, char *buf) > { > static const int values[] = { > SHMEM_HUGE_ALWAYS, Why? > @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj, > SHMEM_HUGE_DENY, > SHMEM_HUGE_FORCE, > }; > - int i, count; > - > - for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) { > - const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s "; > + int len = 0; > + int i; Better: int i, len = 0; > - count += sprintf(buf + count, fmt, > - shmem_format_huge(values[i])); > + for (i = 0; i < ARRAY_SIZE(values); i++) { > + len += sysfs_emit_at(buf, len, > + shmem_huge == values[i] ? "%s[%s]" : "%s%s", > + i ? " " : "", > + shmem_format_huge(values[i])); This is ... complicated. I thought the point of doing all the sysfs_emit stuff was to simplify things.
On Sun, 2020-11-01 at 20:48 +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 12:12:51PM -0800, Joe Perches wrote: > > @@ -4024,7 +4024,7 @@ int __init shmem_init(void) > > > > > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS) > > static ssize_t shmem_enabled_show(struct kobject *kobj, > > - struct kobj_attribute *attr, char *buf) > > + struct kobj_attribute *attr, char *buf) > > { > > static const int values[] = { > > SHMEM_HUGE_ALWAYS, > > Why? why what? > > @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj, > > SHMEM_HUGE_DENY, > > SHMEM_HUGE_FORCE, > > }; > > - int i, count; > > - > > - for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) { > > - const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s "; > > + int len = 0; > > + int i; > > Better: > int i, len = 0; I generally disagree as I think it better to have each declaration on an individual line. > > - count += sprintf(buf + count, fmt, > > - shmem_format_huge(values[i])); > > + for (i = 0; i < ARRAY_SIZE(values); i++) { > > + len += sysfs_emit_at(buf, len, > > + shmem_huge == values[i] ? "%s[%s]" : "%s%s", > > + i ? " " : "", > > + shmem_format_huge(values[i])); > > This is ... complicated. I thought the point of doing all the sysfs_emit > stuff was to simplify things. The removal of fmt allows the format and argument to be __printf verified. Indirected formats do not generally allow that. And using sysfs_emit is not really intended to simplify output code, it's used to make sure there isn't a overflow of the PAGE_SIZE output buf when using sprintf/snprintf.
On Sun, Nov 01, 2020 at 01:04:35PM -0800, Joe Perches wrote: > On Sun, 2020-11-01 at 20:48 +0000, Matthew Wilcox wrote: > > On Sun, Nov 01, 2020 at 12:12:51PM -0800, Joe Perches wrote: > > > @@ -4024,7 +4024,7 @@ int __init shmem_init(void) > > > > > > > > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS) > > > static ssize_t shmem_enabled_show(struct kobject *kobj, > > > - struct kobj_attribute *attr, char *buf) > > > + struct kobj_attribute *attr, char *buf) > > > { > > > static const int values[] = { > > > SHMEM_HUGE_ALWAYS, > > > > Why? > > why what? Why did you change this? > > > @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj, > > > SHMEM_HUGE_DENY, > > > SHMEM_HUGE_FORCE, > > > }; > > > - int i, count; > > > - > > > - for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) { > > > - const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s "; > > > + int len = 0; > > > + int i; > > > > Better: > > int i, len = 0; > > I generally disagree as I think it better to have each declaration on an > individual line. You're wrong. > > > - count += sprintf(buf + count, fmt, > > > - shmem_format_huge(values[i])); > > > + for (i = 0; i < ARRAY_SIZE(values); i++) { > > > + len += sysfs_emit_at(buf, len, > > > + shmem_huge == values[i] ? "%s[%s]" : "%s%s", > > > + i ? " " : "", > > > + shmem_format_huge(values[i])); > > > > This is ... complicated. I thought the point of doing all the sysfs_emit > > stuff was to simplify things. > > The removal of fmt allows the format and argument to be __printf verified. > Indirected formats do not generally allow that. > > And using sysfs_emit is not really intended to simplify output code, it's > used to make sure there isn't a overflow of the PAGE_SIZE output buf when > using sprintf/snprintf. Look, this isn't performance sensitive code. Just do something simple. if (shmem_huge == values[i]) buf += sysfs_emit(buf, "[%s]", shmem_format_huge(values[i])); else buf += sysfs_emit(buf, "%s", shmem_format_huge(values[i])); if (i == ARRAY_SIZE(values) - 1) buf += sysfs_emit(buf, "\n"); else buf += sysfs_emit(buf, " "); Shame there's no sysfs_emitc, but there you go.
On Sun, 2020-11-01 at 21:19 +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 01:04:35PM -0800, Joe Perches wrote: > > On Sun, 2020-11-01 at 20:48 +0000, Matthew Wilcox wrote: > > > On Sun, Nov 01, 2020 at 12:12:51PM -0800, Joe Perches wrote: > > > > @@ -4024,7 +4024,7 @@ int __init shmem_init(void) > > > > > > > > > > > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS) > > > > static ssize_t shmem_enabled_show(struct kobject *kobj, > > > > - struct kobj_attribute *attr, char *buf) > > > > + struct kobj_attribute *attr, char *buf) > > > > { > > > > static const int values[] = { > > > > SHMEM_HUGE_ALWAYS, > > > > > > Why? > > > > why what? > > Why did you change this? Are you asking about the function argument alignment or the commit message? The function argument alignment because the function is being updated. The commit itself because sysfs_emit is the documented preferred interface. > > > > @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj, > > > > SHMEM_HUGE_DENY, > > > > SHMEM_HUGE_FORCE, > > > > }; > > > > - int i, count; > > > > - > > > > - for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) { > > > > - const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s "; > > > > + int len = 0; > > > > + int i; > > > > > > Better: > > > int i, len = 0; > > > > I generally disagree as I think it better to have each declaration on an > > individual line. > > You're wrong. I'm not wrong. We just disagree. Look for yourself at typical declaration use in the kernel. The typical style is single line declarations. That single declaration per line style is also documented in coding-style.rst "To this end, use just one data declaration per line (no commas for multiple data declarations)" > Look, this isn't performance sensitive code. Just do something simple. > > if (shmem_huge == values[i]) > buf += sysfs_emit(buf, "[%s]", > shmem_format_huge(values[i])); > else > buf += sysfs_emit(buf, "%s", > shmem_format_huge(values[i])); > if (i == ARRAY_SIZE(values) - 1) > buf += sysfs_emit(buf, "\n"); > else > buf += sysfs_emit(buf, " "); > > Shame there's no sysfs_emitc, but there you go. I think what's there is simple. And your suggested code doesn't work. sysfs_emit is used for single emits. sysfs_emit_at is used for multiple emits.
On Sun, Nov 01, 2020 at 01:43:13PM -0800, Joe Perches wrote: > > Why did you change this? > > Are you asking about the function argument alignment or the commit message? The indentation. Don't change the fucking indentation, Joe. > > Look, this isn't performance sensitive code. Just do something simple. > > > > if (shmem_huge == values[i]) > > buf += sysfs_emit(buf, "[%s]", > > shmem_format_huge(values[i])); > > else > > buf += sysfs_emit(buf, "%s", > > shmem_format_huge(values[i])); > > if (i == ARRAY_SIZE(values) - 1) > > buf += sysfs_emit(buf, "\n"); > > else > > buf += sysfs_emit(buf, " "); > > > > Shame there's no sysfs_emitc, but there you go. > > I think what's there is simple. Again, you're wrong. > And your suggested code doesn't work. > sysfs_emit is used for single emits. > sysfs_emit_at is used for multiple emits. Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't page aligned. Greg, how about this? +++ b/fs/sysfs/file.c @@ -722,13 +722,13 @@ int sysfs_emit(char *buf, const char *fmt, ...) { va_list args; int len; + int start = offset_in_page(buf); - if (WARN(!buf || offset_in_page(buf), - "invalid sysfs_emit: buf:%p\n", buf)) + if (WARN(!buf, "invalid sysfs_emit: buf:%p\n", buf)) return 0; va_start(args, fmt); - len = vscnprintf(buf, PAGE_SIZE, fmt, args); + len = vscnprintf(buf, PAGE_SIZE - start, fmt, args); va_end(args); return len;
On Sun, 2020-11-01 at 22:06 +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 01:43:13PM -0800, Joe Perches wrote: > > > Why did you change this? > > > > Are you asking about the function argument alignment or the commit message? > > The indentation. Don't change the fucking indentation, Joe. You've being an ass.
On Sun, 2020-11-01 at 22:06 +0000, Matthew Wilcox wrote: > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > page aligned. Greg, how about this? > > +++ b/fs/sysfs/file.c > @@ -722,13 +722,13 @@ int sysfs_emit(char *buf, const char *fmt, ...) > { > va_list args; > int len; > + int start = offset_in_page(buf); I thought of that originally and didn't want to implement it that way as it allows sysfs_emit to be used on non-page aligned buffers. Using two functions makes sure the buf is always page aligned and so buf should never defectively be passed in unaligned.
On Sun, 2020-11-01 at 22:06 +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 01:43:13PM -0800, Joe Perches wrote: > > > Why did you change this? > > > > Are you asking about the function argument alignment or the commit message? > > The indentation. Don't change the fucking indentation, Joe. > > > > Look, this isn't performance sensitive code. Just do something simple. > > > > > > if (shmem_huge == values[i]) > > > buf += sysfs_emit(buf, "[%s]", > > > shmem_format_huge(values[i])); > > > else > > > buf += sysfs_emit(buf, "%s", > > > shmem_format_huge(values[i])); > > > if (i == ARRAY_SIZE(values) - 1) > > > buf += sysfs_emit(buf, "\n"); > > > else > > > buf += sysfs_emit(buf, " "); > > > > > > Shame there's no sysfs_emitc, but there you go. > > > > I think what's there is simple. > > Again, you're wrong. > > > And your suggested code doesn't work. > > sysfs_emit is used for single emits. > > sysfs_emit_at is used for multiple emits. > > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > page aligned. Greg, how about this? > > +++ b/fs/sysfs/file.c > @@ -722,13 +722,13 @@ int sysfs_emit(char *buf, const char *fmt, ...) > { > va_list args; > int len; > + int start = offset_in_page(buf); > > > - if (WARN(!buf || offset_in_page(buf), > - "invalid sysfs_emit: buf:%p\n", buf)) > + if (WARN(!buf, "invalid sysfs_emit: buf:%p\n", buf)) > return 0; > > > va_start(args, fmt); > - len = vscnprintf(buf, PAGE_SIZE, fmt, args); > + len = vscnprintf(buf, PAGE_SIZE - start, fmt, args); > va_end(args); > > > return len; >
On Sun, Nov 01, 2020 at 10:06:04PM +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 01:43:13PM -0800, Joe Perches wrote: > > > Why did you change this? > > > > Are you asking about the function argument alignment or the commit message? > > The indentation. Don't change the fucking indentation, Joe. > > > > Look, this isn't performance sensitive code. Just do something simple. > > > > > > if (shmem_huge == values[i]) > > > buf += sysfs_emit(buf, "[%s]", > > > shmem_format_huge(values[i])); > > > else > > > buf += sysfs_emit(buf, "%s", > > > shmem_format_huge(values[i])); > > > if (i == ARRAY_SIZE(values) - 1) > > > buf += sysfs_emit(buf, "\n"); > > > else > > > buf += sysfs_emit(buf, " "); > > > > > > Shame there's no sysfs_emitc, but there you go. > > > > I think what's there is simple. > > Again, you're wrong. > > > And your suggested code doesn't work. > > sysfs_emit is used for single emits. > > sysfs_emit_at is used for multiple emits. > > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > page aligned. Greg, how about this? How can sysfs_emit() be called on a non-page-aligned buffer? It's being used on the buffer that was passed to the sysfs call. And if you are writing multiple values to a single sysfs file output, well, not good... > > +++ b/fs/sysfs/file.c > @@ -722,13 +722,13 @@ int sysfs_emit(char *buf, const char *fmt, ...) > { > va_list args; > int len; > + int start = offset_in_page(buf); > > - if (WARN(!buf || offset_in_page(buf), > - "invalid sysfs_emit: buf:%p\n", buf)) > + if (WARN(!buf, "invalid sysfs_emit: buf:%p\n", buf)) > return 0; > > va_start(args, fmt); > - len = vscnprintf(buf, PAGE_SIZE, fmt, args); > + len = vscnprintf(buf, PAGE_SIZE - start, fmt, args); That's a bit too 'tricky' for my taste, why is it somehow needed? thanks, greg k-h
On Mon, Nov 02, 2020 at 02:33:43PM +0100, Greg Kroah-Hartman wrote: > > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > > page aligned. Greg, how about this? > > How can sysfs_emit() be called on a non-page-aligned buffer? It's being > used on the buffer that was passed to the sysfs call. > > And if you are writing multiple values to a single sysfs file output, > well, not good... See shmem_enabled_show() in mm/shmem.c (output at /sys/kernel/mm/transparent_hugepage/shmem_enabled on your machine). I don't claim it's a good interface, but it exists.
On Mon, Nov 02, 2020 at 02:08:36PM +0000, Matthew Wilcox wrote: > On Mon, Nov 02, 2020 at 02:33:43PM +0100, Greg Kroah-Hartman wrote: > > > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > > > page aligned. Greg, how about this? > > > > How can sysfs_emit() be called on a non-page-aligned buffer? It's being > > used on the buffer that was passed to the sysfs call. > > > > And if you are writing multiple values to a single sysfs file output, > > well, not good... > > See shmem_enabled_show() in mm/shmem.c (output at > /sys/kernel/mm/transparent_hugepage/shmem_enabled on your machine). > > I don't claim it's a good interface, but it exists. Ok, that's a common pattern for sysfs files, not that bad. What's wrong with using sysfs_emit_at()? We want sysfs_emit() to "know" that the buffer is PAGE_SIZE big, if you try to allow offsets in it, that defeats the purpose of the check. Or am I missing something else here? thanks, greg k-h
On Mon, Nov 02, 2020 at 03:32:59PM +0100, Greg Kroah-Hartman wrote: > On Mon, Nov 02, 2020 at 02:08:36PM +0000, Matthew Wilcox wrote: > > On Mon, Nov 02, 2020 at 02:33:43PM +0100, Greg Kroah-Hartman wrote: > > > > Oh, ugh, sysfs_emit() should be able to work on a buffer that isn't > > > > page aligned. Greg, how about this? > > > > > > How can sysfs_emit() be called on a non-page-aligned buffer? It's being > > > used on the buffer that was passed to the sysfs call. > > > > > > And if you are writing multiple values to a single sysfs file output, > > > well, not good... > > > > See shmem_enabled_show() in mm/shmem.c (output at > > /sys/kernel/mm/transparent_hugepage/shmem_enabled on your machine). > > > > I don't claim it's a good interface, but it exists. > > Ok, that's a common pattern for sysfs files, not that bad. > > What's wrong with using sysfs_emit_at()? We want sysfs_emit() to "know" > that the buffer is PAGE_SIZE big, if you try to allow offsets in it, > that defeats the purpose of the check. For someone who's used to C "strings", it's pretty common to do something like: buf += sprintf(buf, "foo "); buf += sprintf(buf, "bar "); sysfs_emit_at instead wants me to do: len += sprintf(buf + len, "foo "); len += sprintf(buf + len, "bar "); I don't see how the code I wrote defeats the check. It checks that the buffer never crosses a PAGE_SIZE boundary, which is equivalently safe.
On Mon, 2020-11-02 at 14:40 +0000, Matthew Wilcox wrote: > For someone who's used to C "strings", it's pretty common to do > something like: > > buf += sprintf(buf, "foo "); > buf += sprintf(buf, "bar "); It's not equivalent code. What was actually necessary was using scnprintf which tests the number of bytes available in the buffer. The actual equivalent code was something like: int used = 0; used += scnprintf(buf + used, PAGE_SIZE - used, "foo "); used += scnprintf(buf + used, PAGE_SIZE - used, "bar "); > sysfs_emit_at instead wants me to do: > > len += sprintf(buf + len, "foo "); > len += sprintf(buf + len, "bar "); > > I don't see how the code I wrote defeats the check. It checks that the > buffer never crosses a PAGE_SIZE boundary, which is equivalently safe. And it'd be required to store the original buf value passed to be able to return the actual number of bytes emitted when multiple calls are used. return sprintf(buf) - obuf; This also does and can not verify that buf is originally page aligned.
diff --git a/mm/shmem.c b/mm/shmem.c index 5009d783d954..294ba0017a0f 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -4024,7 +4024,7 @@ int __init shmem_init(void) #if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_SYSFS) static ssize_t shmem_enabled_show(struct kobject *kobj, - struct kobj_attribute *attr, char *buf) + struct kobj_attribute *attr, char *buf) { static const int values[] = { SHMEM_HUGE_ALWAYS, @@ -4034,16 +4034,19 @@ static ssize_t shmem_enabled_show(struct kobject *kobj, SHMEM_HUGE_DENY, SHMEM_HUGE_FORCE, }; - int i, count; - - for (i = 0, count = 0; i < ARRAY_SIZE(values); i++) { - const char *fmt = shmem_huge == values[i] ? "[%s] " : "%s "; + int len = 0; + int i; - count += sprintf(buf + count, fmt, - shmem_format_huge(values[i])); + for (i = 0; i < ARRAY_SIZE(values); i++) { + len += sysfs_emit_at(buf, len, + shmem_huge == values[i] ? "%s[%s]" : "%s%s", + i ? " " : "", + shmem_format_huge(values[i])); } - buf[count - 1] = '\n'; - return count; + + len += sysfs_emit_at(buf, len, "\n"); + + return len; } static ssize_t shmem_enabled_store(struct kobject *kobj,
Update the function to use sysfs_emit_at while neatening the uses of sprintf and overwriting the last space char with a newline. Signed-off-by: Joe Perches <joe@perches.com> --- mm/shmem.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)