Re: [PATCH v2 3/5] passt, tap: Add --fd option

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 17 Nov 2022 16:02:43 +0000
"Richard W.M. Jones" <rjones@redhat.com> wrote:

> On Thu, Nov 17, 2022 at 04:49:31PM +0100, Stefano Brivio wrote:
> > On Thu, 17 Nov 2022 15:33:06 +0000
> > "Richard W.M. Jones" <rjones@redhat.com> wrote:
> >   
> > > On Thu, Nov 17, 2022 at 04:26:40PM +0100, Stefano Brivio wrote:  
> > > > From: "Richard W.M. Jones" <rjones@redhat.com>
> > > > 
> > > > This passes a fully connected stream socket to passt.
> > > > 
> > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > > > [sbrivio: reuse fd_tap instead of adding a new descriptor,
> > > >  imply --one-off on --fd, add to optstring and usage()]
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > > ---
> > > > v2:
> > > >  - reuse fd_tap, we don't need a separate file descriptor
> > > >  - add F to optstring and usage(), for both passt and pasta
> > > >  - imply --one-off, we can't do much once the socket is closed
> > > > 
> > > > With this, the trick from 5/5 goes a bit further: passt reads
> > > > from the file descriptor passed by the wrapper.    
> > > 
> > > Thanks for the v2 .. I'll add it to my series and play with it.
> > >   
> > > > However, we get EPOLLRDHUP right away, from the close() on
> > > > one end of the socket pair I guess. Should we just ignore
> > > > all EPOLLRDHUP events, just the first one...?    
> > > 
> > > Does it see the event out-of-band or does it get an in-band
> > > read(2) == 0 after finishing reading the data?  
> > 
> > Out-of-band, so to speak: we won't even recv() if we get EPOLLRDHUP
> > (that's handled in tap_handler()). If I do this on top of this patch:
> > 
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1073,7 +1073,7 @@ void tap_sock_init(struct ctx *c)
> >                         struct epoll_event ev = { 0 };
> >  
> >                         ev.data.fd = c->fd_tap;
> > -                       ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
> > +                       ev.events = EPOLLIN | EPOLLET;
> > 
> > Then it gets those four bytes:
> > 
> > [pid 2538704] epoll_wait(5, 0x7ffedc4a6320, 8, 1000) = 1
> > [pid 2538704] recvfrom(4, 0x560797677000, 8323069, MSG_DONTWAIT, NULL, NULL) = 4
> > [pid 2538704] epoll_wait(5, [], 8, 1000) = 0
> > [pid 2538704] epoll_wait(5, 0x7ffedc4a6320, 8, 1000) = -1 EINTR (Interrupted system call)
> > 
> > and does nothing with them, as expected. Two epoll_wait() calls later,
> > the syscall is interrupted, I'm not sure why and how we should react (in
> > main(), passt.c) in that case.  
> 
> With EPOLLRDHUP removed it's a bit deadlock-y.  One case I commonly
> see is:
> 
> child (qemu):
> 
> 1295637 write(5, "\0\0\0\0", 4 <unfinished ...>
> 1295637 <... write resumed>)            = 4
> 1295637 poll([{fd=5, events=POLLIN|POLLOUT}], 1, -1 <unfinished ...>
> 1295637 <... poll resumed>)             = 1 ([{fd=5, revents=POLLOUT}])
> 1295637 read(3,  <unfinished ...>
> 1295637 <... read resumed>"", 512)      = 0
> 1295637 shutdown(5, SHUT_WR <unfinished ...>
> 1295637 <... shutdown resumed>)         = 0
> 1295637 poll([{fd=5, events=POLLIN}], 1, -1 <unfinished ...>
> 
> then later the parent (passt):
> 
> 1295636 epoll_wait(5, 0x7ffe93a894e0, 8, 1000) = 1
> 1295636 recvfrom(4, 0x5576383cf000, 8323069, MSG_DONTWAIT, NULL, NULL) = 4
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> 1295636 epoll_wait(5, [], 8, 1000)      = 0
> (forever)

Ah, yes, we ignore EPOLLRDHUP, and there's a one-second timeout there,
so it's actually expected.

> Removing EPOLLET (edge triggered) delivers the EPOLLIN event over and
> over again to passt as expected:
> 
> 1299436 recvfrom(4, "", 8323069, MSG_DONTWAIT, NULL, NULL) = 0
> 1299436 epoll_wait(5, 0x7ffdd8a65640, 8, 1000) = 1
> 1299436 recvfrom(4, "", 8323069, MSG_DONTWAIT, NULL, NULL) = 0
> 1299436 epoll_wait(5, 0x7ffdd8a65640, 8, 1000) = 1
> 1299436 recvfrom(4, "", 8323069, MSG_DONTWAIT, NULL, NULL) = 0
> (forever)

I'm not sure that's EPOLLIN, it could be anything (we don't check
explicitly).

> but I expected that passt would exit the first time it reads 0 from
> the socket.
> 
> tap_handler_passt() seems like it only considers the n<0 (error) and
> n>0 (data) cases.  What do you think about checking for n == 0 and  
> exiting immediately if c->one_off is true?

I think it's reasonable in any case. At the same time, we don't
necessarily need to ignore EPOLLRDHUP, we could also simply try to read
from the socket first, that is, move this:

	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)))
		goto reinit;

before this:

	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
		goto reinit;

and we'll just get EBADF on recv(). If we have data to read, we won't
necessarily get a EPOLLRDHUP (or EPOLLHUP) and EPOLLIN separately, so
trying to read first makes sense for the case you described.

An explicit check on EPOLLIN is probably a good idea too: read if we
have anything to read, first, then quit if we have a reason to do so.

-- 
Stefano