Re: [PATCH v1 2/2] epoll: Use __user_write_access_begin() and unsafe_put_user() in epoll_put_uevent().

Kuniyuki Iwashima
2025-10-24T05:16:55Z

From: Dave Hansen <dave.hansen@intel.com>
Date: Thu, 23 Oct 2025 12:40:59 -0700
> On 10/22/25 17:04, Kuniyuki Iwashima wrote:
> > --- a/include/linux/eventpoll.h
> > +++ b/include/linux/eventpoll.h
> > @@ -82,11 +82,14 @@ static inline struct epoll_event __user *
> > epoll_put_uevent(__poll_t revents, __u64 data,
> > struct epoll_event __user *uevent)
> > {
> > - if (__put_user(revents, &uevent->events) ||
> > - __put_user(data, &uevent->data))
> > - return NULL;
> > -
> > - return uevent+1;
> > + __user_write_access_begin(uevent, sizeof(*uevent));
> > + unsafe_put_user(revents, &uevent->events, efault);
> > + unsafe_put_user(data, &uevent->data, efault);
> > + user_access_end();
> > + return uevent + 1;
> > +efault:
> > + user_access_end();
> > + return NULL;
> > }
> > #endif
>
> This makes me nervous. The access_ok() check is quite a distance away.
> I'd kinda want to see some performance numbers before doing this. Is
> removing a single access_ok() even measurable?

I noticed I made a typo in commit message, s/tcp_rr/udp_rr/.

epoll_put_uevent() can be called multiple times in a single
epoll_wait(), and we can see 1.7% more pps on UDP even when
1 thread has 1000 sockets only:

server: $ udp_rr --nolog -6 -F 1000 -T 1 -l 3600
client: $ udp_rr --nolog -6 -F 1000 -T 256 -l 3600 -c -H $SERVER
server: $ nstat > /dev/null; sleep 10; nstat | grep -i udp

Without patch (2 stac/clac):
Udp6InDatagrams 2205209 0.0

With patch (1 stac/clac):
Udp6InDatagrams 2242602 0.0

>>> 2242602 / 2205209 * 100
101.6956669413194

I also took a microbenchmark with bpftrace and we can see
more invocations of ep_try_send_events_ns() finish faster,
and 4% more total calls:

$ sudo bpftrace -e '
k:ep_try_send_events { @start[cpu] = nsecs; }
kr:ep_try_send_events {
if (@start[cpu]) {
$delay = nsecs - @start[cpu];
delete(@start[cpu]);
@ep_try_send_events_ns = hist($delay);
}
}
END { clear(@start); }' -c 'sleep 10'

Without patch:

@ep_try_send_events_ns:
[256, 512) 2483257 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[512, 1K) 850735 |@@@@@@@@@@@@@@@@@ |
[1K, 2K) 254027 |@@@@@ |
[2K, 4K) 26646 | |
[4K, 8K) 1358 | |
[8K, 16K) 66 | |
[16K, 32K) 3 | |

With patch:

@ep_try_send_events_ns:
[256, 512) 2844733 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[512, 1K) 733956 |@@@@@@@@@@@@@ |
[1K, 2K) 166349 |@@@ |
[2K, 4K) 13495 | |
[4K, 8K) 526 | |
[8K, 16K) 63 | |
[16K, 32K) 5 | |

>>> (2844733 + 733956 + 166349 + 13495 + 526 + 63 + 5) / \
... (2483257 + 850735 + 254027 + 26646 + 1358 + 66 + 3) * 100
103.95551329999347

>
> Also, even if we go do this, shouldn't __user_write_access_begin() be
> called something more like unsafe_user_write_access_begin()?

Sounds good.