Re: [RFC v2 11/11] tcp_buf: adding TCP tx circular buffer

[Eugenio Perez Martin <eperezma@redhat.com>]

On Thu, Jul 24, 2025 at 3:34 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Wed, Jul 09, 2025 at 07:47:48PM +0200, Eugenio Pérez wrote:
> > Now both tcp_sock and tap uses the circular buffer as intended.
> >
> > Very lightly tested. Especially, paths like ring full or almost full
> > that are checked before producing like
> > tcp_payload_sock_used + fill_bufs > TCP_FRAMES_MEM.
> >
> > Processing the tx buffers in a circular buffer makes namespace rx go
> > from to ~11.5Gbit/s. to ~17.26Gbit/s.
> >
> > TODO: Increase the tx queue length, as we spend a lot of descriptors in
> > each request. Ideally, tx size should be at least
> > bufs_per_frame*TCP_FRAMES_MEM, but maybe we got more performance with
> > bigger queues.
> >
> > TODO: Sometimes we call tcp_buf_free_old_tap_xmit twice: one to free at
> > least N used tx buffers and the next one in tcp_payload_flush. Maybe we
> > can optimize it.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  tcp_buf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 106 insertions(+), 24 deletions(-)
> >
> > diff --git a/tcp_buf.c b/tcp_buf.c
> > index f74d22d..326af79 100644
> > --- a/tcp_buf.c
> > +++ b/tcp_buf.c
> > @@ -53,13 +53,66 @@ static_assert(MSS6 <= sizeof(tcp_payload[0].data), "MSS6 is greater than 65516")
> >
> >  /* References tracking the owner connection of frames in the tap outqueue */
> >  static struct tcp_tap_conn *tcp_frame_conns[TCP_FRAMES_MEM];
> > -static unsigned int tcp_payload_sock_used, tcp_payload_tap_used;
> > +
> > +/*
> > + * sock_head: Head of buffers available for writing. tcp_data_to_tap moves it
> > + * forward, but errors queueing to vhost can move it backwards to tap_head
> > + * again.
> > + *
> > + * tap_head: Head of buffers that have been sent to vhost. flush moves this
> > + * forward.
> > + *
> > + * tail: Chasing index. Increments when vhost uses buffers.
> > + *
> > + * _used: Independent variables to tell between full and empty.
>
> Hm.  I kind of hope there's a less bulky way of doing this.
>

The other option I know is to always keep one entry unused.

> > + */
> > +static unsigned int tcp_payload_sock_head, tcp_payload_tap_head, tcp_payload_tail, tcp_payload_sock_used, tcp_payload_tap_used;
> > +#define IS_POW2(y) (((y) > 0) && !((y) & ((y) - 1)))
>
> Worth putting this in util.h as a separate patch.
>

Agree.

> > +static_assert(ARRAY_SIZE(tcp_payload) == TCP_FRAMES_MEM, "TCP_FRAMES_MEM is not the size of tcp_payload anymore");
> > +static_assert(IS_POW2(TCP_FRAMES_MEM), "TCP_FRAMES_MEM must be a power of two");
> > +
> > +static size_t tcp_payload_cnt_to_end(size_t head, size_t tail)
> > +{
> > +     assert(head != tail);
> > +     size_t end = ARRAY_SIZE(tcp_payload) - tail;
> > +     size_t n = (head + end) % ARRAY_SIZE(tcp_payload);
> > +
> > +     return MIN(n, end);
> > +}
> > +
> > +/* Count the number of items that has been written from sock to the
> > + * curcular buffer and can be sent to tap.
>
> s/curcular/circular/g
>

Thanks for the catch, fixing in the next release!

> > + */
> > +static size_t tcp_payload_tap_cnt(void)
> > +{
> > +     return tcp_payload_sock_used - tcp_payload_tap_used;
> > +}
> >
> >  static void tcp_payload_sock_produce(size_t n)
> >  {
> > +     tcp_payload_sock_head = (tcp_payload_sock_head + n) % ARRAY_SIZE(tcp_payload);
> >       tcp_payload_sock_used += n;
> >  }
> >
> > +/* Count the number of consecutive items that has been written from sock to the
> > + * curcular buffer and can be sent to tap without having to wrap back to the
> > + * beginning of the buffer.
> > + */
> > +static size_t tcp_payload_tap_cnt_to_end(void)
> > +{
> > +     if (tcp_payload_sock_head == tcp_payload_tap_head) {
> > +             /* empty? */
> > +             if (tcp_payload_sock_used - tcp_payload_tap_used == 0)
> > +                     return 0;
> > +
> > +             /* full */
> > +             return ARRAY_SIZE(tcp_payload) - tcp_payload_tap_head;
> > +     }
> > +
> > +     return tcp_payload_cnt_to_end(tcp_payload_sock_head,
> > +                                   tcp_payload_tap_head);
> > +}
> > +
> >  static struct iovec  tcp_l2_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS];
> >
> >  /**
> > @@ -137,14 +190,13 @@ static void tcp_revert_seq(const struct ctx *c, struct tcp_tap_conn **conns,
> >       }
> >  }
> >
> > -static void tcp_buf_free_old_tap_xmit(const struct ctx *c)
> > +static void tcp_buf_free_old_tap_xmit(const struct ctx *c, size_t target)
> >  {
> > -     while (tcp_payload_tap_used) {
> > -             tap_free_old_xmit(c, tcp_payload_tap_used);
> > +       size_t n = tap_free_old_xmit(c, target);
> >
> > -             tcp_payload_tap_used = 0;
> > -             tcp_payload_sock_used = 0;
> > -     }
> > +       tcp_payload_tail = (tcp_payload_tail + n) & (ARRAY_SIZE(tcp_payload) - 1);
>
> use % instead of & here - it's consistent with other places, and the
> compiler should be able to optimize it to the same thing.
>
> > +       tcp_payload_tap_used -= n;
> > +       tcp_payload_sock_used -= n;
> >  }
> >
> >  /**
> > @@ -153,16 +205,33 @@ static void tcp_buf_free_old_tap_xmit(const struct ctx *c)
> >   */
> >  void tcp_payload_flush(const struct ctx *c)
> >  {
> > -     size_t m;
> > +     size_t m, n = tcp_payload_tap_cnt_to_end();
> > +     struct iovec *head = &tcp_l2_iov[tcp_payload_tap_head][0];
> >
> > -     m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS,
> > -                         tcp_payload_sock_used, true);
> > -     if (m != tcp_payload_sock_used) {
> > -             tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m],
> > -                            tcp_payload_sock_used - m);
> > -     }
> > +     tcp_buf_free_old_tap_xmit(c, (size_t)-1);
> > +     m = tap_send_frames(c, head, TCP_NUM_IOVS, n, true);
> >       tcp_payload_tap_used += m;
> > -     tcp_buf_free_old_tap_xmit(c);
> > +     tcp_payload_tap_head = (tcp_payload_tap_head + m) %
> > +                            ARRAY_SIZE(tcp_payload);
> > +
> > +     if (m != n) {
> > +             n = tcp_payload_tap_cnt_to_end();
> > +
> > +             tcp_revert_seq(c, &tcp_frame_conns[tcp_payload_tap_head],
> > +                            &tcp_l2_iov[tcp_payload_tap_head], n);
> > +             /*
> > +              * circular buffer wrap case.
> > +              * TODO: Maybe it's better to adapt tcp_revert_seq.
> > +              */
> > +             tcp_revert_seq(c, &tcp_frame_conns[0], &tcp_l2_iov[0],
> > +                            tcp_payload_tap_cnt() - n);
> > +
> > +             tcp_payload_sock_head = tcp_payload_tap_head;
> > +             tcp_payload_sock_used = tcp_payload_tap_used;
> > +     } else if (tcp_payload_tap_cnt_to_end()) {
> > +             /* circular buffer wrap case */
> > +             tcp_payload_flush(c);
> > +     }
> >  }
> >
> >  /**
> > @@ -209,14 +278,15 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
> >       size_t optlen;
> >       size_t l4len;
> >       uint32_t seq;
> > +     unsigned int i = tcp_payload_sock_head;
> >       int ret;
> >
> > -     iov = tcp_l2_iov[tcp_payload_sock_used];
> > +     iov = tcp_l2_iov[i];
> >       if (CONN_V4(conn)) {
> > -             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_sock_used]);
> > +             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[i]);
> >               iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> >       } else {
> > -             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_sock_used]);
> > +             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[i]);
> >               iov[TCP_IOV_ETH].iov_base = &tcp6_eth_src;
> >       }
> >
> > @@ -228,13 +298,15 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
> >               return ret;
> >
> >       tcp_payload_sock_produce(1);
> > +     i = tcp_payload_sock_head;
> >       l4len = optlen + sizeof(struct tcphdr);
> >       iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> >       tcp_l2_buf_fill_headers(conn, iov, NULL, seq, false);
> >
> >       if (flags & DUP_ACK) {
> > -             struct iovec *dup_iov = tcp_l2_iov[tcp_payload_sock_used];
> > +             struct iovec *dup_iov = tcp_l2_iov[i];
> >               tcp_payload_sock_produce(1);
> > +             i = tcp_payload_sock_head;
> >
> >               memcpy(dup_iov[TCP_IOV_TAP].iov_base, iov[TCP_IOV_TAP].iov_base,
> >                      iov[TCP_IOV_TAP].iov_len);
> > @@ -246,7 +318,10 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
> >       }
> >
> >       if (tcp_payload_sock_used > TCP_FRAMES_MEM - 2) {
> > +             tcp_buf_free_old_tap_xmit(c, 2);
> >               tcp_payload_flush(c);
> > +             /* TODO how to fix this? original code didn't chech for success either */
> > +             assert(tcp_payload_sock_used <= TCP_FRAMES_MEM - 2);
> >       }
> >
> >       return 0;
> > @@ -269,16 +344,17 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >       struct iovec *iov;
> >
> >       conn->seq_to_tap = seq + dlen;
> > -     tcp_frame_conns[tcp_payload_sock_used] = conn;
> > -     iov = tcp_l2_iov[tcp_payload_sock_used];
> > +     tcp_frame_conns[tcp_payload_sock_head] = conn;
> > +     iov = tcp_l2_iov[tcp_payload_sock_head];
> >       if (CONN_V4(conn)) {
> >               if (no_csum) {
> > -                     struct iovec *iov_prev = tcp_l2_iov[tcp_payload_sock_used - 1];
> > +                     unsigned prev = (tcp_payload_sock_head - 1) % TCP_FRAMES_MEM;
> > +                     struct iovec *iov_prev = tcp_l2_iov[prev];
> >                       struct iphdr *iph = iov_prev[TCP_IOV_IP].iov_base;
> >
> >                       check = &iph->check;
> >               }
> > -             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_sock_used]);
> > +             iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_sock_head]);
> >               iov[TCP_IOV_ETH].iov_base = &tcp4_eth_src;
> >       } else if (CONN_V6(conn)) {
> >               iov[TCP_IOV_IP] = IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_sock_used]);
> > @@ -294,8 +370,11 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn,
> >       tcp_l2_buf_fill_headers(conn, iov, check, seq, false);
> >       tcp_payload_sock_produce(1);
> >       if (tcp_payload_sock_used > TCP_FRAMES_MEM - 1) {
> > +             tcp_buf_free_old_tap_xmit(c, 1);
> >               tcp_payload_flush(c);
> > +             assert(tcp_payload_sock_used <= TCP_FRAMES_MEM - 1);
> >       }
> > +
> >  }
> >
> >  /**
> > @@ -362,11 +441,14 @@ int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
> >       }
> >
> >       if (tcp_payload_sock_used + fill_bufs > TCP_FRAMES_MEM) {
> > +             tcp_buf_free_old_tap_xmit(c, fill_bufs);
> >               tcp_payload_flush(c);
> > +             /* TODO how to report this to upper layers? */
> > +             assert(tcp_payload_sock_used + fill_bufs <= TCP_FRAMES_MEM);
> >       }
> >
> >       for (i = 0, iov = iov_sock + 1; i < fill_bufs; i++, iov++) {
> > -             iov->iov_base = &tcp_payload[tcp_payload_sock_used + i].data;
> > +             iov->iov_base = &tcp_payload[(tcp_payload_sock_head + i) % TCP_FRAMES_MEM].data;
> >               iov->iov_len = mss;
> >       }
> >       if (iov_rem)
>
> --
> David Gibson (he or they)       | 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