On Sun, Aug 16, 2026 at 08:39:29PM +0300, Ammar Yasser wrote:
> On Thu Aug 13, 2026 at 4:16 AM EEST, David Gibson wrote:
> > On Wed, Aug 12, 2026 at 08:17:19PM +0300, Ammar Yasser wrote:
> >> On Mon Aug 10, 2026 at 5:06 AM EEST, David Gibson wrote:
> >> >>  
> >> >>  /* Large enough for ~128 maximum size frames */
> >> >> -#define PKT_BUF_BYTES		(8UL << 20)
> >> >> +#define PKT_BUF_BYTES		((8UL << 20) + 1536)	/* 128 * sizeof(virtio_net_hdr_mrg_rxbuf) */
> >> >
> >> > I think the rationale for this change needs to be clearer (granted,
> >> > the comment here beforehand is also kind of confusing).  IIRC - and
> >> > based on the "~" in the comment, I don't think there's a strict
> >> > requirement that this can hold 128 full frames - that's just setting a
> >> > reasonable sense of scale, and then a round number was picked near it:
> >> > ~64kiB * ~64 ~= 8MiB
> >> >
> >> > So, I'm not sure if this change is necessary - if it really is, we
> >> > need a clearer analysis of why.
> >> 
> >> Because pkt_buf is now going to be the buffer where vhost guest->pasta
> >> data but with the added size of the virtio_net_hdr_mrg_rxbuf for every
> >> frame. So this is accounting for the worst case where the guest wants to
> >> send a full 128 frames at maximum size at a time.
> >
> > Right, but that's not enough.  AFAICT pkt_buf is sized to allow
> > *roughly* 128 full packets, but it doesn't strictly have to be able to
> > contain that many.  If there's a reason it *must* have room for 128
> > full frames with vhost-kernel, that needs to be pointed out
> > explicitly.
> 
> Are you saying the size is not enough or the explanation is not ?

Sorry, I meant the explanation.

> Also no, there's no explicit reason why it needs to have that many
> packets in vhost-kernel. I saw that this was how pkt_buf was sized
> before the vhost-kernel changes so i only added acounting for the virtio
> net header

Right, that's the point I'm making.  I think having an aligned size
fot the whole buffer is probably more valuable than being able to
contain exactly 128 frames.

> >> >> +#define VHOST_NDESCS (PKT_BUF_BYTES / 65520)
> >> >
> >> > I'm not sure 65520 is the right number here.  That's the max MTU at
> >> > the IP level, but the packet buffer will also hold the 14 byte L2
> >> > header.  I think you probably want one of the L2_MAX_LEN_* constants
> >> > (or to define a new one for vhost-kernel).
> >> 
> >> My line of reasoning here is that "we typically expect, in the majority
> >> of cases for an ethernet frame to span a single descriptor". evident by
> >> how we eventually consume descriptors as we return the pointer into the
> >> pkt_buf past the virtio_net header as the beginning of an ethernet
> >> header. and this buffer should handle 128 frames (from the definition 
> >> of PKT_BUF_BYTES) and so we want a denominator that yields a value as
> >> close as possible to 128. Let me know if this isn't very sound. I will
> >> investiagate the constants you mentioned anyways
> >
> > If the fundamental property you want is that you can fit 128
> > descriptors, then you should just define NDESCS as 128, and derive the
> > buffer size and other things from that.  If the basic property you
> > want is something else, define that first and the rest in terms of it.
> 
> Fair enough, can do that 
> 

-- 
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