Messages in this thread | | | Date | Sun, 1 Nov 2020 21:19:10 +0000 | From | Matthew Wilcox <> | Subject | Re: [PATCH 4/5] mm: shmem: Convert shmem_enabled_show to use sysfs_emit_at |
| |
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.
| |