On Wed, 26 Mar 2025 14:44:04 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Share some additional miscellaneous logic between the vhost-user and "buf" paths for data on udp reply sockets. The biggest piece is error handling of cases where we can't forward between the two pifs of the flow. We also make common some more simple logic locating the correct flow and its parameters. This adds some lines of code due to extra comment lines, but nonetheless reduces logic duplication. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- udp.c | 41 ++++++++++++++++++++++++++--------------- udp_vu.c | 26 +++++++++++--------------- udp_vu.h | 3 ++- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/udp.c b/udp.c index 55021ac4..4258812e 100644 --- a/udp.c +++ b/udp.c @@ -754,24 +754,25 @@ void udp_listen_sock_handler(const struct ctx *c, /** * udp_buf_reply_sock_data() - Handle new data from flow specific socket * @c: Execution context - * @ref: epoll reference + * @s: Socket to read data from + * @tosidx: Flow & side to forward data from @s to * @now: Current timestamp * + * Return: true on success, false if can't forward from socket to flow's pif"on success"...+ * * #syscalls recvmmsg */ -static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref, +static bool udp_buf_reply_sock_data(const struct ctx *c, + int s, flow_sidx_t tosidx, const struct timespec *now) { - flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); const struct flowside *toside = flowside_at_sidx(tosidx); - struct udp_flow *uflow = udp_at_sidx(ref.flowside); + struct udp_flow *uflow = udp_at_sidx(tosidx); uint8_t topif = pif_at_sidx(tosidx); - int n, i, from_s; - - from_s = uflow->s[ref.flowside.sidei]; + int n, i; - if ((n = udp_sock_recv(c, from_s, udp_mh_recv)) <= 0) - return; + if ((n = udp_sock_recv(c, s, udp_mh_recv)) <= 0) + return true;is not exactly accurate: 0 means either no datagrams (should never happen) or error on recvmmsg(). But I think it's clear enough, and you'll have follow-ups anyway, so I went ahead and applied this. Maybe: * Return: false if we can't forward from socket to flow's pif, true otherwise ? Actually, looking at the caller:flow_trace(uflow, "Received %d datagrams on reply socket", n); uflow->ts = now->tv_sec; @@ -790,11 +791,10 @@ static void udp_buf_reply_sock_data(const struct ctx *c, union epoll_ref ref, } else if (topif == PIF_TAP) { tap_send_frames(c, &udp_l2_iov[0][0], UDP_NUM_IOVS, n); } else { - uint8_t frompif = pif_at_sidx(ref.flowside); - - flow_err(uflow, "No support for forwarding UDP from %s to %s", - pif_name(frompif), pif_name(topif)); + return false; } + + return true; } /** @@ -821,10 +821,21 @@ void udp_reply_sock_handler(const struct ctx *c, union epoll_ref ref, } if (events & EPOLLIN) { + flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); + int s = ref.fd; + bool ret; + if (c->mode == MODE_VU) - udp_vu_reply_sock_data(c, ref, now); + ret = udp_vu_reply_sock_data(c, s, tosidx, now); else - udp_buf_reply_sock_data(c, ref, now); + ret = udp_buf_reply_sock_data(c, s, tosidx, now); + + if (!ret) {the opposite would probably be less surprising, if (ret) flow_err(...).+ flow_err(uflow, + "No support for forwarding UDP from %s to %s", + pif_name(pif_at_sidx(ref.flowside)), + pif_name(pif_at_sidx(tosidx))); + } } } diff --git a/udp_vu.c b/udp_vu.c index 6e1823a9..06bdeae6 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -273,38 +273,32 @@ void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref, /** * udp_vu_reply_sock_data() - Handle new data from flow specific socket * @c: Execution context - * @ref: epoll reference + * @s: Socket to read data from + * @tosidx: Flow & side to forward data from @s to * @now: Current timestamp + * + * Return: true on success, false if can't forward from socket to flow's pif */ -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx, const struct timespec *now) { - flow_sidx_t tosidx = flow_sidx_opposite(ref.flowside); const struct flowside *toside = flowside_at_sidx(tosidx); bool v6 = !(inany_v4(&toside->eaddr) && inany_v4(&toside->oaddr)); - struct udp_flow *uflow = udp_at_sidx(ref.flowside); - int from_s = uflow->s[ref.flowside.sidei]; + struct udp_flow *uflow = udp_at_sidx(tosidx); struct vu_dev *vdev = c->vdev; struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE]; - uint8_t topif = pif_at_sidx(tosidx); int i; ASSERT(uflow); - if (topif != PIF_TAP) { - uint8_t frompif = pif_at_sidx(ref.flowside); - - flow_err(uflow, - "No support for forwarding UDP from %s to %s", - pif_name(frompif), pif_name(topif)); - return; - } + if (pif_at_sidx(tosidx) != PIF_TAP) + return false; for (i = 0; i < UDP_MAX_FRAMES; i++) { ssize_t dlen; int iov_used; - iov_used = udp_vu_sock_recv(c, from_s, v6, &dlen); + iov_used = udp_vu_sock_recv(c, s, v6, &dlen); if (iov_used <= 0) break; flow_trace(uflow, "Received 1 datagram on reply socket"); @@ -318,4 +312,6 @@ void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, } vu_flush(vdev, vq, elem, iov_used); } + + return true; } diff --git a/udp_vu.h b/udp_vu.h index 4f2262d0..2299b51f 100644 --- a/udp_vu.h +++ b/udp_vu.h @@ -8,6 +8,7 @@ void udp_vu_listen_sock_data(const struct ctx *c, union epoll_ref ref, const struct timespec *now); -void udp_vu_reply_sock_data(const struct ctx *c, union epoll_ref ref, +bool udp_vu_reply_sock_data(const struct ctx *c, int s, flow_sidx_t tosidx, const struct timespec *now); + #endif /* UDP_VU_H */-- Stefano