Re: [PATCH 4/5] mm: shmem: Convert shmem_enabled_show to use sysfs_emit_at

[Joe Perches]

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.