[PATCH] fs/select.c: batch user writes in do_sys_poll

[PATCH] fs/select.c: batch user writes in do_sys_poll

[Daniel Axtens]

When returning results to userspace, do_sys_poll repeatedly calls
put_user() - once per fd that it's watching.

This means that on architectures that support some form of
kernel-to-userspace access protection, we end up enabling and disabling
access once for each file descripter we're watching. This is inefficent
and we can improve things by batching the accesses together.

To make sure there's not too much happening in the window when user
accesses are permitted, we don't walk the linked list with accesses on.
This leads to some slightly messy code in the loop, unfortunately.

Unscientific benchmarking with the poll2_threads microbenchmark from
will-it-scale, run as `./poll2_threads -t 1 -s 15`:

 - Bare-metal Power9 with KUAP: ~48.8% speed-up
 - VM on amd64 laptop with SMAP: ~25.5% speed-up

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 fs/select.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 7aef49552d4c..f58976da9e63 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -1015,9 +1015,19 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 		struct pollfd *fds = walk->entries;
 		int j;
 
+		if (!user_write_access_begin(ufds, (sizeof(struct pollfd) *
+						    walk->len)))
+			goto out_fds;
+
 		for (j = 0; j < walk->len; j++, ufds++)
-			if (__put_user(fds[j].revents, &ufds->revents))
-				goto out_fds;
+			unsafe_put_user(fds[j].revents, &ufds->revents, loop_fault);
+
+		user_write_access_end();
+		continue;
+
+loop_fault:
+		user_write_access_end();
+		goto out_fds;
   	}
 
 	err = fdcount;
-- 
2.25.1

Re: [PATCH] fs/select.c: batch user writes in do_sys_poll

[Christoph Hellwig]

On Thu, Aug 13, 2020 at 05:11:20PM +1000, Daniel Axtens wrote:
> When returning results to userspace, do_sys_poll repeatedly calls
> put_user() - once per fd that it's watching.
> 
> This means that on architectures that support some form of
> kernel-to-userspace access protection, we end up enabling and disabling
> access once for each file descripter we're watching. This is inefficent
> and we can improve things by batching the accesses together.
> 
> To make sure there's not too much happening in the window when user
> accesses are permitted, we don't walk the linked list with accesses on.
> This leads to some slightly messy code in the loop, unfortunately.
> 
> Unscientific benchmarking with the poll2_threads microbenchmark from
> will-it-scale, run as `./poll2_threads -t 1 -s 15`:
> 
>  - Bare-metal Power9 with KUAP: ~48.8% speed-up
>  - VM on amd64 laptop with SMAP: ~25.5% speed-up
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>

Seem like this could simply use a copy_to_user to further simplify
things?

Also please don't pointlessly add overly long lines.

Re: [PATCH] fs/select.c: batch user writes in do_sys_poll

[Daniel Axtens]

Christoph Hellwig <hch@infradead.org> writes:

> On Thu, Aug 13, 2020 at 05:11:20PM +1000, Daniel Axtens wrote:
>> When returning results to userspace, do_sys_poll repeatedly calls
>> put_user() - once per fd that it's watching.
>> 
>> This means that on architectures that support some form of
>> kernel-to-userspace access protection, we end up enabling and disabling
>> access once for each file descripter we're watching. This is inefficent
>> and we can improve things by batching the accesses together.
>> 
>> To make sure there's not too much happening in the window when user
>> accesses are permitted, we don't walk the linked list with accesses on.
>> This leads to some slightly messy code in the loop, unfortunately.
>> 
>> Unscientific benchmarking with the poll2_threads microbenchmark from
>> will-it-scale, run as `./poll2_threads -t 1 -s 15`:
>> 
>>  - Bare-metal Power9 with KUAP: ~48.8% speed-up
>>  - VM on amd64 laptop with SMAP: ~25.5% speed-up
>> 
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>
> Seem like this could simply use a copy_to_user to further simplify
> things?

I'll benchmark it and find out.

> Also please don't pointlessly add overly long lines.

Weird, I ran the commit through checkpatch and it didn't pick it
up. I'll check the next version more carefully.

Regards,
Daniel

Re: [PATCH] fs/select.c: batch user writes in do_sys_poll

[Daniel Axtens]

Hi,

>> Seem like this could simply use a copy_to_user to further simplify
>> things?
>
> I'll benchmark it and find out.

I tried this:

        for (walk = head; walk; walk = walk->next) {
-               struct pollfd *fds = walk->entries;
-               int j;
-
-               for (j = 0; j < walk->len; j++, ufds++)
-                       if (__put_user(fds[j].revents, &ufds->revents))
-                               goto out_fds;
+               if (copy_to_user(ufds, walk->entries,
+                                sizeof(struct pollfd) * walk->len))
+                       goto out_fds;
+               ufds += walk->len;
        }

With that approach, the poll2 microbenchmark (which polls 128 fds) is
about as fast as v1.

However, the poll1 microbenchmark, which polls just 1 fd, regresses a
touch (<1% - ~2%) compared to the current code, although it's largely
within the noise. Thoughts?

Kind regards,
Daniel

>> Also please don't pointlessly add overly long lines.
>
> Weird, I ran the commit through checkpatch and it didn't pick it
> up. I'll check the next version more carefully.
>
> Regards,
> Daniel

RE: [PATCH] fs/select.c: batch user writes in do_sys_poll

[David Laight]

From: Daniel Axtens
> Sent: 13 August 2020 12:37
> 
> >> Seem like this could simply use a copy_to_user to further simplify
> >> things?
> >
> > I'll benchmark it and find out.
> 
> I tried this:
> 
>         for (walk = head; walk; walk = walk->next) {
> -               struct pollfd *fds = walk->entries;
> -               int j;
> -
> -               for (j = 0; j < walk->len; j++, ufds++)
> -                       if (__put_user(fds[j].revents, &ufds->revents))
> -                               goto out_fds;
> +               if (copy_to_user(ufds, walk->entries,
> +                                sizeof(struct pollfd) * walk->len))
> +                       goto out_fds;
> +               ufds += walk->len;
>         }
> 
> With that approach, the poll2 microbenchmark (which polls 128 fds) is
> about as fast as v1.
> 
> However, the poll1 microbenchmark, which polls just 1 fd, regresses a
> touch (<1% - ~2%) compared to the current code, although it's largely
> within the noise. Thoughts?

Is that with or without 'user copy hardening'?
Or use __copy_to_user() to skip all that 'crap'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] fs/select.c: batch user writes in do_sys_poll

[Al Viro]

On Thu, Aug 13, 2020 at 09:36:52PM +1000, Daniel Axtens wrote:
> Hi,
> 
> >> Seem like this could simply use a copy_to_user to further simplify
> >> things?
> >
> > I'll benchmark it and find out.
> 
> I tried this:
> 
>         for (walk = head; walk; walk = walk->next) {
> -               struct pollfd *fds = walk->entries;
> -               int j;
> -
> -               for (j = 0; j < walk->len; j++, ufds++)
> -                       if (__put_user(fds[j].revents, &ufds->revents))
> -                               goto out_fds;
> +               if (copy_to_user(ufds, walk->entries,
> +                                sizeof(struct pollfd) * walk->len))
> +                       goto out_fds;
> +               ufds += walk->len;
>         }
> 
> With that approach, the poll2 microbenchmark (which polls 128 fds) is
> about as fast as v1.
> 
> However, the poll1 microbenchmark, which polls just 1 fd, regresses a
> touch (<1% - ~2%) compared to the current code, although it's largely
> within the noise. Thoughts?

I'd go with copy_to_user() here; post such variant and I'll throw it into
-next after -rc1.

RE: [PATCH] fs/select.c: batch user writes in do_sys_poll

[David Laight]

From: Christoph Hellwig
> Sent: 13 August 2020 08:32
> 
> On Thu, Aug 13, 2020 at 05:11:20PM +1000, Daniel Axtens wrote:
> > When returning results to userspace, do_sys_poll repeatedly calls
> > put_user() - once per fd that it's watching.
> >
> > This means that on architectures that support some form of
> > kernel-to-userspace access protection, we end up enabling and disabling
> > access once for each file descripter we're watching. This is inefficent
> > and we can improve things by batching the accesses together.
> >
> > To make sure there's not too much happening in the window when user
> > accesses are permitted, we don't walk the linked list with accesses on.
> > This leads to some slightly messy code in the loop, unfortunately.
> >
> > Unscientific benchmarking with the poll2_threads microbenchmark from
> > will-it-scale, run as `./poll2_threads -t 1 -s 15`:
> >
> >  - Bare-metal Power9 with KUAP: ~48.8% speed-up
> >  - VM on amd64 laptop with SMAP: ~25.5% speed-up
> >
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> Seem like this could simply use a copy_to_user to further simplify
> things?

That would copy out 8 bytes/fd instead of 2.
So a slight change for 32bit kernels.
However the 'user copy hardening' checks that copy_to_user()
does probably make a measurable difference.

> Also please don't pointlessly add overly long lines.

Shorten the error label?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)