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

[Matthew Wilcox]

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.