Re: [PATCH 05/11] tcp_splice: Avoid awkward temporaries in tcp_splice_epoll_ctl()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Oct 2023 12:51:08 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> We initialise the events_a and events_b variables with
> tcp_splice_conn_epoll_events() function, then immediately copy the values
> into ev_a.events and ev_b.events.  We can't simply pass &ev_[ab].events to
> tcp_splice_conn_epoll_events(), because struct epoll_event is packed,
> leading to 'pointer may be unaligned' warnings if we attempt that.
> 
> We can, however, make tcp_splice_conn_epoll_events() take struct
> epoll_event pointers rather than raw u32 pointers, avoiding the awkward
> temporaries.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp_splice.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 439fc1d..3419207 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -95,25 +95,26 @@ static int tcp_sock_refill_ns(void *arg);
>  /**
>   * tcp_splice_conn_epoll_events() - epoll events masks for given state
>   * @events:	Connection event flags
> - * @a:		Event mask for socket with accepted connection, set on return
> - * @b:		Event mask for connection target socket, set on return
> + * @a:		Event for socket with accepted connection, set on return
> + * @b:		Event for connection target socket, set on return
>   */
>  static void tcp_splice_conn_epoll_events(uint16_t events,
> -					 uint32_t *a, uint32_t *b)
> +					 struct epoll_event *a,
> +					 struct epoll_event *b)
>  {
> -	*a = *b = 0;
> +	a->events = b->events = 0;
>  
>  	if (events & SPLICE_ESTABLISHED) {
>  		if (!(events & B_FIN_SENT))
> -			*a = EPOLLIN | EPOLLRDHUP;
> +			a->events = EPOLLIN | EPOLLRDHUP;
>  		if (!(events & A_FIN_SENT))
> -			*b = EPOLLIN | EPOLLRDHUP;
> +			b->events = EPOLLIN | EPOLLRDHUP;
>  	} else if (events & SPLICE_CONNECT) {
> -		*b = EPOLLOUT;
> +		b->events = EPOLLOUT;
>  	}
>  
> -	*a |= (events & A_OUT_WAIT) ? EPOLLOUT : 0;
> -	*b |= (events & B_OUT_WAIT) ? EPOLLOUT : 0;
> +	a->events |= (events & A_OUT_WAIT) ? EPOLLOUT : 0;
> +	a->events |= (events & B_OUT_WAIT) ? EPOLLOUT : 0;

This should be b->events |= ..., but it's fixed in 7/11 anyway.

-- 
Stefano