Re: [PATCH 3/3] tap: Don't size pool_tap[46] for the maximum number of packets

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 13 Dec 2024 23:01:56 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we attempt to size pool_tap[46] so they have room for the maximum
> possible number of packets that could fit in pkt_buf, TAP_MSGS.  However,
> the calculation isn't quite correct: TAP_MSGS is based on ETH_ZLEN (60) as
> the minimum possible L2 frame size.  But, we don't enforce that L2 frames
> are at least ETH_ZLEN when we receive them from the tap backend, and since
> we're dealing with virtual interfaces we don't have the physical Ethernet
> limitations requiring that length.  Indeed it is possible to generate a
> legitimate frame smaller than that (e.g. a zero-payload UDP/IPv4 frame on
> the 'pasta' backend is only 42 bytes long).
> 
> It's also unclear if this limit is sufficient for vhost-user which isn't
> limited by the size of pkt_buf as the other modes are.
> 
> We could attempt to correct the calculation, but that would leave us with
> even larger arrays, which in practice rarely accumulate more than a handful
> of packets.  So, instead, put an arbitrary cap on the number of packets we
> can put in a batch, and if we run out of space, process and flush the
> batch.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  packet.c    | 13 ++++++++++++-
>  packet.h    |  3 +++
>  passt.h     |  2 --
>  tap.c       | 18 +++++++++++++++---
>  tap.h       |  3 ++-
>  vu_common.c |  3 ++-
>  6 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index 5bfa7304..b68580cc 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -22,6 +22,17 @@
>  #include "util.h"
>  #include "log.h"
>  
> +/**
> + * pool_full() - Is a packet pool full?
> + * @p:		Pointer to packet pool
> + *
> + * Return: true if the pool is full, false if more packets can be added
> + */
> +bool pool_full(const struct pool *p)
> +{
> +	return p->count >= p->size;
> +}
> +
>  /**
>   * packet_add_do() - Add data as packet descriptor to given pool
>   * @p:		Existing pool
> @@ -35,7 +46,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
>  {
>  	size_t idx = p->count;
>  
> -	if (idx >= p->size) {
> +	if (pool_full(p)) {
>  		trace("add packet index %zu to pool with size %zu, %s:%i",
>  		      idx, p->size, func, line);
>  		return;
> diff --git a/packet.h b/packet.h
> index 98eb8812..3618f213 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -6,6 +6,8 @@
>  #ifndef PACKET_H
>  #define PACKET_H
>  
> +#include <stdbool.h>
> +
>  /**
>   * struct pool - Generic pool of packets stored in nmemory
>   * @size:	Number of usable descriptors for the pool
> @@ -23,6 +25,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
>  void *packet_get_do(const struct pool *p, const size_t idx,
>  		    size_t offset, size_t len, size_t *left,
>  		    const char *func, int line);
> +bool pool_full(const struct pool *p);
>  void pool_flush(struct pool *p);
>  
>  #define packet_add(p, len, start)					\
> diff --git a/passt.h b/passt.h
> index 0dd4efa0..81b2787f 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -70,8 +70,6 @@ static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data),
>  
>  #define TAP_BUF_BYTES							\
>  	ROUND_DOWN(((ETH_MAX_MTU + sizeof(uint32_t)) * 128), PAGE_SIZE)
> -#define TAP_MSGS							\
> -	DIV_ROUND_UP(TAP_BUF_BYTES, ETH_ZLEN - 2 * ETH_ALEN + sizeof(uint32_t))
>  
>  #define PKT_BUF_BYTES		MAX(TAP_BUF_BYTES, 0)
>  extern char pkt_buf		[PKT_BUF_BYTES];
> diff --git a/tap.c b/tap.c
> index 68231f09..42370a26 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -61,6 +61,8 @@
>  #include "vhost_user.h"
>  #include "vu_common.h"
>  
> +#define TAP_MSGS		256

Sorry, I stopped at 2/3, had just a quick look at this one, and I
missed this.

Assuming 4 KiB pages, this changes from 161319 to 256. You mention that
in practice we never have more than a handful of messages, which is
probably almost always the case, but I wonder if that's also the case
with UDP "real-time" streams, where we could have bursts of a few
hundred (thousand?) messages at a time.

I wonder: how bad would it be to correct the calculation, instead? We
wouldn't actually use more memory, right?

-- 
Stefano