From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 1B6145A0271 for ; Wed, 9 Aug 2023 21:59:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691611156; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GrGUy7W1RLYndwmWz1vW10j+P9DYOqUWTBcfuENDlO0=; b=M8BBhX6XOdz7HVV52stq+bxGmu1RyHhbcJEu/47NcYT0/ogKPGSw1GuiZBsMIZv3LPCcfC J0aWElUjLiXTCIlJ5JYgRaoox13oZBBBtr0dcnwc4iKnaCxTINOKoawX9UrI/lib2Lwf+l jkNRINd9uqsKo2AdyDv1QegW6O6aQh8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-467-yPzmw5w-P7aUYkWYSHb5nQ-1; Wed, 09 Aug 2023 15:59:15 -0400 X-MC-Unique: yPzmw5w-P7aUYkWYSHb5nQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AE3928DC666; Wed, 9 Aug 2023 19:59:14 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.5]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7FF8C2166B25; Wed, 9 Aug 2023 19:59:12 +0000 (UTC) Date: Wed, 9 Aug 2023 21:59:10 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/9] epoll: Generalize epoll_ref to cover things other than sockets Message-ID: <20230809215743.22102e88@elisabeth> In-Reply-To: <20230807134631.1400119-2-david@gibson.dropbear.id.au> References: <20230807134631.1400119-1-david@gibson.dropbear.id.au> <20230807134631.1400119-2-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: Q2D3IM4HNT4FHB5QHRAYKHXNYSK52RGG X-Message-ID-Hash: Q2D3IM4HNT4FHB5QHRAYKHXNYSK52RGG X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, 7 Aug 2023 23:46:23 +1000 David Gibson wrote: > The epoll_ref type includes fields for the IP protocol of a socket, and the > socket fd. However, we already have a few things in the epoll which aren't > protocol sockets, and we may have more in future. Rename these fields to > an abstract "fd type" and file descriptor for more generality. > > Similarly, rather than using existing IP protocol numbers for the type, > introduce our own number space. For now these just correspond to the > supported protocols, but we'll expand on that in future. > > Signed-off-by: David Gibson > --- > icmp.c | 6 +++--- > passt.c | 25 ++++++++++++------------- > passt.h | 40 +++++++++++++++++++++++++++++----------- > tcp.c | 22 +++++++++++----------- > tcp_conn.h | 4 ++-- > tcp_splice.c | 4 ++-- > udp.c | 14 +++++++------- > util.c | 27 ++++++++++++++++++++------- > 8 files changed, 86 insertions(+), 56 deletions(-) > > diff --git a/icmp.c b/icmp.c > index 676fa64..a4b6a47 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -79,7 +79,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, > (void)events; > (void)now; > > - n = recvfrom(ref.s, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); > + n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); > if (n < 0) > return; > > @@ -182,7 +182,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, > bind_if, id, iref.u32); > if (s < 0) > goto fail_sock; > - if (s > SOCKET_MAX) { > + if (s > FD_REF_MAX) { > close(s); > return 1; > } > @@ -236,7 +236,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, > bind_if, id, iref.u32); > if (s < 0) > goto fail_sock; > - if (s > SOCKET_MAX) { > + if (s > FD_REF_MAX) { > close(s); > return 1; > } > diff --git a/passt.c b/passt.c > index 9123868..b42f42d 100644 > --- a/passt.c > +++ b/passt.c > @@ -55,12 +55,11 @@ > > char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); > > -char *ip_proto_str[IPPROTO_SCTP + 1] = { > - [IPPROTO_ICMP] = "ICMP", > - [IPPROTO_TCP] = "TCP", > - [IPPROTO_UDP] = "UDP", > - [IPPROTO_ICMPV6] = "ICMPV6", > - [IPPROTO_SCTP] = "SCTP", > +char *epoll_type_str[EPOLL_TYPE_MAX+1] = { For consistency, given you're respinning anyway: [EPOLL_TYPE_MAX + 1] > + [EPOLL_TYPE_TCP] = "TCP socket", > + [EPOLL_TYPE_UDP] = "UDP socket", > + [EPOLL_TYPE_ICMP] = "ICMP socket", > + [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", > }; > > /** > @@ -73,16 +72,16 @@ char *ip_proto_str[IPPROTO_SCTP + 1] = { > static void sock_handler(struct ctx *c, union epoll_ref ref, > uint32_t events, const struct timespec *now) > { > - trace("%s: %s packet from socket %i (events: 0x%08x)", > + trace("%s: packet from %s %i (events: 0x%08x)", > c->mode == MODE_PASST ? "passt" : "pasta", > - IP_PROTO_STR(ref.proto), ref.s, events); > + EPOLL_TYPE_STR(ref.type), ref.fd, events); > > - if (!c->no_tcp && ref.proto == IPPROTO_TCP) > - tcp_sock_handler( c, ref, events, now); > - else if (!c->no_udp && ref.proto == IPPROTO_UDP) > - udp_sock_handler( c, ref, events, now); > + 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.proto == IPPROTO_ICMP || ref.proto == IPPROTO_ICMPV6)) > + (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6)) > icmp_sock_handler(c, ref, events, now); > } > > diff --git a/passt.h b/passt.h > index edc4841..2110781 100644 > --- a/passt.h > +++ b/passt.h > @@ -42,9 +42,27 @@ union epoll_ref; > #include "udp.h" > > /** > - * union epoll_ref - Breakdown of reference for epoll socket bookkeeping > - * @proto: IP protocol number > - * @s: Socket number (implies 2^24-1 limit on number of descriptors) > + * enum epoll_type - Different types of fds we poll over > + */ > +enum epoll_type { > + /* Special value to indicate an invalid type */ > + EPOLL_TYPE_NONE = 0, > + /* Sockets and timerfds for TCP handling */ > + EPOLL_TYPE_TCP, > + /* UDP sockets */ > + EPOLL_TYPE_UDP, > + /* IPv4 ICMP sockets */ > + EPOLL_TYPE_ICMP, > + /* ICMPv6 sockets */ > + EPOLL_TYPE_ICMPV6, > + > + EPOLL_TYPE_MAX = EPOLL_TYPE_ICMPV6, > +}; > + > +/** > + * union epoll_ref - Breakdown of reference for epoll fd bookkeeping > + * @type: Type of fd (tells us what to do with events) > + * @fd: File descriptor number (implies < 2^24 total descriptors) > * @tcp: TCP-specific reference part > * @udp: UDP-specific reference part > * @icmp: ICMP-specific reference part > @@ -53,10 +71,10 @@ union epoll_ref; > */ > union epoll_ref { > struct { > - int32_t proto:8, > -#define SOCKET_REF_BITS 24 > -#define SOCKET_MAX MAX_FROM_BITS(SOCKET_REF_BITS) > - s:SOCKET_REF_BITS; > + enum epoll_type type:8; > +#define FD_REF_BITS 24 > +#define FD_REF_MAX MAX_FROM_BITS(FD_REF_BITS) > + int32_t fd:FD_REF_BITS; > union { > union tcp_epoll_ref tcp; > union udp_epoll_ref udp; > @@ -78,10 +96,10 @@ static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data), > #define PKT_BUF_BYTES MAX(TAP_BUF_BYTES, 0) > extern char pkt_buf [PKT_BUF_BYTES]; > > -extern char *ip_proto_str[]; > -#define IP_PROTO_STR(n) \ > - (((uint8_t)(n) <= IPPROTO_SCTP && ip_proto_str[(n)]) ? \ > - ip_proto_str[(n)] : "?") > +extern char *epoll_type_str[]; > +#define EPOLL_TYPE_STR(n) \ > + (((uint8_t)(n) <= EPOLL_TYPE_MAX && epoll_type_str[(n)]) ? \ > + epoll_type_str[(n)] : "?") > > #include /* For MAXNS below */ > > diff --git a/tcp.c b/tcp.c > index d9ecee5..18b781a 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -643,7 +643,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, > static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn) > { > int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; > - union epoll_ref ref = { .proto = IPPROTO_TCP, .s = conn->sock, > + union epoll_ref ref = { .type = EPOLL_TYPE_TCP, .fd = conn->sock, > .tcp.index = CONN_IDX(conn) }; > struct epoll_event ev = { .data.u64 = ref.u64 }; > > @@ -663,8 +663,8 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn) > conn->c.in_epoll = true; > > if (conn->timer != -1) { > - union epoll_ref ref_t = { .proto = IPPROTO_TCP, > - .s = conn->sock, > + union epoll_ref ref_t = { .type = EPOLL_TYPE_TCP, > + .fd = conn->sock, > .tcp.timer = 1, > .tcp.index = CONN_IDX(conn) }; > struct epoll_event ev_t = { .data.u64 = ref_t.u64, > @@ -692,8 +692,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) > return; > > if (conn->timer == -1) { > - union epoll_ref ref = { .proto = IPPROTO_TCP, > - .s = conn->sock, > + union epoll_ref ref = { .type = EPOLL_TYPE_TCP, > + .fd = conn->sock, > .tcp.timer = 1, > .tcp.index = CONN_IDX(conn) }; > struct epoll_event ev = { .data.u64 = ref.u64, > @@ -701,7 +701,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) > int fd; > > fd = timerfd_create(CLOCK_MONOTONIC, 0); > - if (fd == -1 || fd > SOCKET_MAX) { > + if (fd == -1 || fd > FD_REF_MAX) { > debug("TCP: failed to get timer: %s", strerror(errno)); > if (fd > -1) > close(fd); > @@ -1908,7 +1908,7 @@ int tcp_conn_new_sock(const struct ctx *c, sa_family_t af) > > s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP); > > - if (s > SOCKET_MAX) { > + if (s > FD_REF_MAX) { > close(s); > return -EIO; > } > @@ -2791,7 +2791,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, > * https://github.com/llvm/llvm-project/issues/58992 > */ > memset(&sa, 0, sizeof(struct sockaddr_in6)); > - s = accept4(ref.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); > + s = accept4(ref.fd, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); > if (s < 0) > return; > > @@ -2948,7 +2948,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, > conn = tc + ref.tcp.index; > > if (conn->c.spliced) > - tcp_splice_sock_handler(c, &conn->splice, ref.s, events); > + tcp_splice_sock_handler(c, &conn->splice, ref.fd, events); > else > tcp_tap_sock_handler(c, &conn->tap, events); > } > @@ -2999,7 +2999,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port, > int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, > const char *ifname, in_port_t port) > { > - int r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1; > + int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; > > if (af == AF_UNSPEC && c->ifi4 && c->ifi6) > /* Attempt to get a dual stack socket */ > @@ -3013,7 +3013,7 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, > if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) > r6 = tcp_sock_init_af(c, AF_INET6, port, addr, ifname); > > - if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) > + if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) > return 0; > > return r4 < 0 ? r4 : r6; > diff --git a/tcp_conn.h b/tcp_conn.h > index 9e2b1bf..0b36940 100644 > --- a/tcp_conn.h > +++ b/tcp_conn.h > @@ -62,7 +62,7 @@ struct tcp_tap_conn { > unsigned int ws_to_tap :TCP_WS_BITS; > > > - int sock :SOCKET_REF_BITS; > + int sock :FD_REF_BITS; > > uint8_t events; > #define CLOSED 0 > @@ -80,7 +80,7 @@ struct tcp_tap_conn { > (SOCK_ACCEPTED | TAP_SYN_RCVD | ESTABLISHED) > > > - int timer :SOCKET_REF_BITS; > + int timer :FD_REF_BITS; > > uint8_t flags; > #define STALLED BIT(0) > diff --git a/tcp_splice.c b/tcp_splice.c > index 03e14c1..24995e2 100644 > --- a/tcp_splice.c > +++ b/tcp_splice.c > @@ -173,9 +173,9 @@ static int tcp_splice_epoll_ctl(const struct ctx *c, > struct tcp_splice_conn *conn) > { > int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; > - union epoll_ref ref_a = { .proto = IPPROTO_TCP, .s = conn->a, > + union epoll_ref ref_a = { .type = EPOLL_TYPE_TCP, .fd = conn->a, > .tcp.index = CONN_IDX(conn) }; > - union epoll_ref ref_b = { .proto = IPPROTO_TCP, .s = conn->b, > + union epoll_ref ref_b = { .type = EPOLL_TYPE_TCP, .fd = conn->b, > .tcp.index = CONN_IDX(conn) }; > struct epoll_event ev_a = { .data.u64 = ref_a.u64 }; > struct epoll_event ev_b = { .data.u64 = ref_b.u64 }; > diff --git a/udp.c b/udp.c > index 5a852fb..62f8360 100644 > --- a/udp.c > +++ b/udp.c > @@ -388,9 +388,9 @@ static void udp_sock6_iov_init(const struct ctx *c) > int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) > { > struct epoll_event ev = { .events = EPOLLIN | EPOLLRDHUP | EPOLLHUP }; > - union epoll_ref ref = { .proto = IPPROTO_UDP, > + union epoll_ref ref = { .type = EPOLL_TYPE_UDP, > .udp = { .splice = true, .ns = ns, > - .v6 = v6, .port = src } > + .v6 = v6, .port = src } > }; > struct udp_splice_port *sp; > int act, s; > @@ -406,7 +406,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) > s = socket(v6 ? AF_INET6 : AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, > IPPROTO_UDP); > > - if (s > SOCKET_MAX) { > + if (s > FD_REF_MAX) { > close(s); > return -EIO; > } > @@ -414,7 +414,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) > if (s < 0) > return s; > > - ref.s = s; > + ref.fd = s; > > if (v6) { > struct sockaddr_in6 addr6 = { > @@ -767,7 +767,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, > udp4_localname.sin_port = htons(dstport); > } > > - n = recvmmsg(ref.s, mmh_recv, n, 0, NULL); > + n = recvmmsg(ref.fd, mmh_recv, n, 0, NULL); > if (n <= 0) > return; > > @@ -980,7 +980,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, > const void *addr, const char *ifname, in_port_t port) > { > union udp_epoll_ref uref = { .u32 = 0 }; > - int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1; > + int s, r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; > > if (ns) { > uref.port = (in_port_t)(port + c->udp.fwd_out.f.delta[port]); > @@ -1030,7 +1030,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, > } > } > > - if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) > + if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) > return 0; > > return r4 < 0 ? r4 : r6; > diff --git a/util.c b/util.c > index 019c56c..2cac7ba 100644 > --- a/util.c > +++ b/util.c > @@ -102,7 +102,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, > const void *bind_addr, const char *ifname, uint16_t port, > uint32_t data) > { > - union epoll_ref ref = { .proto = proto, .data = data }; > + union epoll_ref ref = { .data = data }; > struct sockaddr_in addr4 = { > .sin_family = AF_INET, > .sin_port = htons(port), > @@ -118,9 +118,22 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, > int fd, sl, y = 1, ret; > struct epoll_event ev; > > - if (proto != IPPROTO_TCP && proto != IPPROTO_UDP && > - proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6) > + switch (proto) { > + case IPPROTO_TCP: > + ref.type = EPOLL_TYPE_TCP; > + break; > + case IPPROTO_UDP: > + ref.type = EPOLL_TYPE_UDP; > + break; > + case IPPROTO_ICMP: > + ref.type = EPOLL_TYPE_ICMP; > + break; > + case IPPROTO_ICMPV6: > + ref.type = EPOLL_TYPE_ICMPV6; > + break; > + default: > return -EPFNOSUPPORT; /* Not implemented. */ > + } > > if (af == AF_UNSPEC) { > if (!DUAL_STACK_SOCKETS || bind_addr) > @@ -140,12 +153,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, > return ret; > } > > - if (fd > SOCKET_MAX) { > + if (fd > FD_REF_MAX) { > close(fd); > return -EBADF; > } > > - ref.s = fd; > + ref.fd = fd; > > if (af == AF_INET) { > if (bind_addr) > @@ -188,8 +201,8 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, > if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, > ifname, strlen(ifname))) { > ret = -errno; > - warn("Can't bind socket for %s port %u to %s, closing", > - ip_proto_str[proto], port, ifname); > + warn("Can't bind %s socket for port %u to %s, closing", > + EPOLL_TYPE_STR(proto), port, ifname); > close(fd); > return ret; > }