From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202412 header.b=USdAFhF4; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 005DD5A061A for ; Fri, 20 Dec 2024 02:13:40 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202412; t=1734657205; bh=9f6JbnF+PlC56Yy3qMYn4hf2ytBhTTvPrPPs3+vVjvI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=USdAFhF4Mkpaw1Sx3GcEpYypFVfCgm0GZhgzn4BMxwcRHFBIjbMb30+VEoUtXlNoI xq5iVZC62ledLoTc4PLJqcMzTBBDsllmMPcKFhdZSp+N/s5OEwbbztoAMEoBdENml5 ofjbO28jzPrG+DzXPaUaqZNsgIOdYYsDf5h1OQPy7akjsx5YwzJg/bz7UnoVl3XYA2 226VZf36CICF8KPAy+SMuqhE3/78/kAVm6W1/LvnW8BujKJlQ01A2d4dglMCtqdoSd Ck72vgpgFdM5HMDOqy2P37C7872QcoQKyq5iL+lefIXB0Dp244CbdR8xTTsPAr1mJu A8I6DUrOBgrcA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4YDqDP4JbNz4x5g; Fri, 20 Dec 2024 12:13:25 +1100 (AEDT) Date: Fri, 20 Dec 2024 12:13:23 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/3] tap: Don't size pool_tap[46] for the maximum number of packets Message-ID: References: <20241213120156.4123972-1-david@gibson.dropbear.id.au> <20241213120156.4123972-4-david@gibson.dropbear.id.au> <20241219100015.3e4b7599@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="zxlnztsKYo2FzCMk" Content-Disposition: inline In-Reply-To: <20241219100015.3e4b7599@elisabeth> Message-ID-Hash: HQXFGQVZJO7F6XOXBHVU2M5KIOSYJZ7O X-Message-ID-Hash: HQXFGQVZJO7F6XOXBHVU2M5KIOSYJZ7O X-MailFrom: dgibson@gandalf.ozlabs.org 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: --zxlnztsKYo2FzCMk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 19, 2024 at 10:00:15AM +0100, Stefano Brivio wrote: > On Fri, 13 Dec 2024 23:01:56 +1100 > David Gibson wrote: >=20 > > Currently we attempt to size pool_tap[46] so they have room for the max= imum > > possible number of packets that could fit in pkt_buf, TAP_MSGS. Howeve= r, > > 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 fram= es > > are at least ETH_ZLEN when we receive them from the tap backend, and si= nce > > we're dealing with virtual interfaces we don't have the physical Ethern= et > > 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). > >=20 > > 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. > >=20 > > We could attempt to correct the calculation, but that would leave us wi= th > > even larger arrays, which in practice rarely accumulate more than a han= dful > > 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. > >=20 > > Signed-off-by: David Gibson > > --- > > 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(-) > >=20 > > 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" > > =20 > > +/** > > + * 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 >=3D 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 =3D p->count; > > =20 > > - if (idx >=3D 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 > > =20 > > +#include > > + > > /** > > * 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); > > =20 > > #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) <=3D sizeof(uni= on epoll_data), > > =20 > > #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= )) > > =20 > > #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" > > =20 > > +#define TAP_MSGS 256 >=20 > Sorry, I stopped at 2/3, had just a quick look at this one, and I > missed this. >=20 > Assuming 4 KiB pages, this changes from 161319 to 256. You mention that Yes. I'm certainly open to arguments on what the number should be. > 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. Maybe. If we are getting them in large bursts, then we're no longer really suceeding at the streams being "real-time", but sure, we should try to catch up as best we can. > I wonder: how bad would it be to correct the calculation, instead? We > wouldn't actually use more memory, right? I was pretty painful when I tried, and it would use more memory. The safe option would be to use ETH_HLEN as the minimum size (which is pretty much all we enforce in the tap layer), which would expand the iovec array here by 2-3x. It's not enormous, but it's not nothing. Or do you mean the unused pages of the array would never be instantiated? In which case, yeah, I guess not. Remember that with the changes in this patch if we exceed TAP_MSGS, nothing particularly bad happens: we don't crash, and we don't drop packets; we just process things in batches of TAP_MSGS frames at a time. So this doesn't need to be large enough to handle any burst we could ever get, just large enough to adequately mitigate the per-batch costs, which I don't think are _that_ large. 256 was a first guess at that. Maybe it's not enough, but I'd be pretty surprised if it needed to be greater than ~1000 to make the per-batch costs negligible compared to the per-frame costs. UDP_MAX_FRAMES, which is on the reverse path but serves a similar function, is only 32. --=20 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 --zxlnztsKYo2FzCMk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmdkxLIACgkQzQJF27ox 2GeVHQ/+Kncg2KrY16kRkKBJohNz9chxiLLe6S6jWeuZjqoDYDTGS51SiL1aU/cU dD5h5sBK2gpRpeksFVt1wXsyNgaj/7jbzazh34wUHaEu5cg/Zx55JueDciqYZlKb jkB2zb8yAExJlW1dLq4vi1PuOfGz6iSnKe0B+dyPH5YdOvOD1C05LrgiikgGNiLY AWQu82jU6ZiXriXezgOF/NWStqoQr5HbwpGoacU4MqnN+Kp2ezKD3XOVnqeD3m8L Mwk2+6VOZDX3o9TiuOhhGU4vSrgNyJl5WTJobWRSVI5sX9wflTPNbQB8lNo8LfX8 dH1xARpz9TjBA7cT+fUEJ7Bx6iG4ZcHym7M+Rb65Srx4d6pgektVGkFfFE5GPq0N WqGADNowVMpfO8xW7tMjtcof+LgWzGWraBjTXhJcZFiNMdTnv+88nTCcjc5yMeSa ip0WIGd/f0hLtV6cvNoCMH4sXZ4V0vRky/iBBsiRJel651YDCQg4JOza3e634Czy VbKreqf08Mfof7+Y+R2u4COjuQ/bUjv5DQ56i04g2Sy6Q47vfyRVCGWQ0IRZrl8f ylMxRDWVygS34brP1QymZyBzWzIH7UNdSZscglfqpa7NDbC6K/gjf/avSwOUQ+9N tyXcPUNlcStJ9aqAFZaRlQ7ccaMGy5iWORz/ydILlh7t9SbQWho= =SjY+ -----END PGP SIGNATURE----- --zxlnztsKYo2FzCMk--