Re: [PATCH v2 07/13] epoll: Fold sock_handler into general switch on epoll event fd

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

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

On Thu, Aug 10, 2023 at 09:49:37PM +0200, Stefano Brivio wrote:
> On Thu, 10 Aug 2023 12:33:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > When we process events from epoll_wait(), we check for a number of special
> > cases before calling sock_handler() which then dispatches based on the
> > protocol type of the socket in the event.
> > 
> > Fold these cases together into a single switch on the fd type recorded in
> > the epoll data field.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  passt.c | 54 +++++++++++++++++++++++++++---------------------------
> >  1 file changed, 27 insertions(+), 27 deletions(-)
> > 
> > diff --git a/passt.c b/passt.c
> > index 45e9fbf..665262b 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -64,29 +64,6 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = {
> >  	[EPOLL_TYPE_TAP]	= "tap device",
> >  };
> >  
> > -/**
> > - * sock_handler() - Event handler for L4 sockets
> > - * @c:		Execution context
> > - * @ref:	epoll reference
> > - * @events:	epoll events
> > - * @now:	Current timestamp
> > - */
> > -static void sock_handler(struct ctx *c, union epoll_ref ref,
> > -			 uint32_t events, const struct timespec *now)
> > -{
> > -	trace("%s: packet from %s %i (events: 0x%08x)",
> > -	      c->mode == MODE_PASST ? "passt" : "pasta",
> > -	      EPOLL_TYPE_STR(ref.type), ref.fd, events);
> > -
> > -	if (!c->no_tcp && ref.type == EPOLL_TYPE_TCP)
> > -		tcp_sock_handler(c, ref, events, now);
> > -	else if (!c->no_udp && ref.type == EPOLL_TYPE_UDP)
> > -		udp_sock_handler(c, ref, events, now);
> > -	else if (!c->no_icmp &&
> > -		 (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6))
> > -		icmp_sock_handler(c, ref, events, now);
> > -}
> > -
> >  /**
> >   * post_handler() - Run periodic and deferred tasks for L4 protocol handlers
> >   * @c:		Execution context
> > @@ -330,13 +307,36 @@ loop:
> >  
> >  	for (i = 0; i < nfds; i++) {
> >  		union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64);
> > +		uint32_t eventmask = events[i].events;
> >  
> > -		if (ref.type == EPOLL_TYPE_TAP)
> > +		trace("%s: epoll event on %s %i (events: 0x%08x)",
> > +		      c.mode == MODE_PASST ? "passt" : "pasta",
> > +		      EPOLL_TYPE_STR(ref.type), ref.fd, events);
> 
> Gah, sorry I missed this earlier, but covscan reported it -- you
> probably wanted to pass 'eventmask' to trace().

Oops, good catch.  Fixed for a new spin

> Actually, a long time ago, I was pondering about a macro that would
> print constants' names ("EPOLLIN", etc.) instead, but then I thought
> that once you remember the values from <sys/epoll.h>, hex values might
> be a bit easier on eyes when you're digging through thousands of
> them... or maybe not. I don't know actually.

I'm inclined to leave it as hex for simplicity.  The fact that we
could have multiple events means it's not really simple macro
territory; we'd have to format a bunch of stuff.  I don't think that's
worth bothering with for a trace message unless we repeatedly find
it's making our debugging more difficult, which hasn't happened so
far.

-- 
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 --]