diff mbox series

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

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

Commit Message

Joe Perches Nov. 1, 2020, 8:12 p.m. UTC
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(-)

Comments

Matthew Wilcox Nov. 1, 2020, 8:48 p.m. UTC | #1
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.
Joe Perches Nov. 1, 2020, 9:04 p.m. UTC | #2
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.
Matthew Wilcox Nov. 1, 2020, 9:19 p.m. UTC | #3
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.
Joe Perches Nov. 1, 2020, 9:43 p.m. UTC | #4
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.
Matthew Wilcox Nov. 1, 2020, 10:06 p.m. UTC | #5
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;
Joe Perches Nov. 1, 2020, 10:08 p.m. UTC | #6
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.
Joe Perches Nov. 1, 2020, 10:13 p.m. UTC | #7
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.
Joe Perches Nov. 1, 2020, 10:14 p.m. UTC | #8
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;
>
Greg KH Nov. 2, 2020, 1:33 p.m. UTC | #9
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
Matthew Wilcox Nov. 2, 2020, 2:08 p.m. UTC | #10
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.
Greg KH Nov. 2, 2020, 2:32 p.m. UTC | #11
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
Matthew Wilcox Nov. 2, 2020, 2:40 p.m. UTC | #12
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.
Joe Perches Nov. 2, 2020, 5:01 p.m. UTC | #13
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 mbox series

Patch

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,