On Fri, Oct 23, 2026 at 11:04 AM Michael S. Tsirkin <[email protected]> wrote: > > Great patch, thanks! > > On Fri, Oct 23, 2026 at 01:19:19AM -0700, Ruben Hakobyan wrote: > > The virtio_balloon driver reports a scatter-gather list of free pages > > to the VMM so it may discard unused pages. ... > > It's not just a question of convenient, is it? > Rust is buggy wrt to spec as it allows only U32_MAX. > So it is also to make it work. > So should we maybe fix Rust to be spec compliant and allow 2^32 too? > > I think we should definitely recommend U32_MAX in the spec > because otherwise it can not fit in the used length. >
Thanks for the review! Sounds good to me, I will CC [email protected] as per your recommendation. Agreed that Rust-VMM should also be fixed in case 2^32 needs to stay the hard limit. I think here for the driver, we can be more conservative as long as we stay below 2^32. ... > > --- > > The wording in the spec is a bit ambiguous because it uses "chain > > length" for both descriptor counts and the total size of the memory > > ranges referenced by the descriptors. My read is that the byte limit > > applies to the sum of descriptor lengths, which is also what rust-vmm > > checks. > > > quote what you are referring to please, also in the commit log > not after ---. I will update with quotes in v2, thanks. > > I would also CC [email protected] - spec should recommend > U32_MAX. > > > > > Another option I considered was to limit reporting capacity based > > on the largest possible block. E.g. for 64 KiB pages, the largest > > block is 512 MiB, so limit the reporting capacity to 7 to stay > > below 4GiB. Although simpler, I found that the time to reclaim > > was quite a bit longer. > > > > So I was going to apply this actually: > > https://lore.kernel.org/all/b541ff170218bd5d45b551b5132e7cf2c490d761.1780906288.git....@redhat.com/ > > which makes the question completely moot. > > Does this have any effect on performance? > > Thanks! Thanks for sharing this. Yes, this would also make sure we stay below the limit. Regarding performance, I tested this on QEMU and it calls virtio_notify() after each completion. Using one chain per entry creates more chains, so QEMU can use more CPU time to reclaim the same amount of memory. If we go with this, I think a QEMU patch to batch notifications could reduce the overhead. Alternatively, I think we could create larger chains here but make sure they stay within the 2^32 limit, and keeping your approach of queuing them all before kicking. I am happy to share a patch with my thinking here. ... > > - if (WARN_ON_ONCE(err)) > > - return err; > > + /* > > + * Virtio spec 2.7.5.2 > > Please add spec version and chapter title, and quote the > spec text in question verbatim. Will update, thanks for the note. > > > limits the total buffer length of a > > + * descriptor chain to 2^32 bytes (4 GiB), so split larger > > + * reports into batches no bigger than that size. > > + */ > > + while (batch_nents < nents && > > + sg->length <= U32_MAX - batch_len) { > > here you actually limit to U32_MAX which is 2^32 - 1 as commit log says. I will update the comment above to be more explicit about this. ...
