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

[Kuniyuki Iwashima <kuniyu@google.com>]

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.