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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2642 bytes --]

On Fri, Nov 03, 2023 at 05:21:34PM +0100, Stefano Brivio wrote:
> 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.

Oops.  Fixed, nonetheless.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]