On Mon, May 23, 2022 at 04:49:45PM +0100, Alexandru Elisei wrote: > Hi, > > On Mon, May 23, 2022 at 04:13:23PM +0100, Andre Przywara wrote: > > On Mon, 23 May 2022 15:06:23 +0000 > > Keir Fraser <keirf@xxxxxxxxxx> wrote: > > > > > On Mon, May 23, 2022 at 03:42:49PM +0100, Andre Przywara wrote: > > > > On Fri, 20 May 2022 21:51:07 +0100 > > > > Will Deacon <will@xxxxxxxxxx> wrote: > > > > > > > > Hi, > > > > > > > > > On Fri, 20 May 2022 14:37:04 +0000, Keir Fraser wrote: > > > > > > While playing with kvmtool's virtio_balloon device I found a couple of > > > > > > niggling issues with the printing of memory stats. Please consider > > > > > > these fairly trivial fixes. > > > > > > > > Unfortunately patch 2/2 breaks compilation on userland with older kernel > > > > headers, like Ubuntu 18.04: > > > > ... > > > > CC builtin-stat.o > > > > builtin-stat.c: In function 'do_memstat': > > > > builtin-stat.c:86:8: error: 'VIRTIO_BALLOON_S_HTLB_PGALLOC' undeclared (first use in this function); did you mean 'VIRTIO_BALLOON_S_AVAIL'? > > > > case VIRTIO_BALLOON_S_HTLB_PGALLOC: > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > VIRTIO_BALLOON_S_AVAIL > > > > (repeated for VIRTIO_BALLOON_S_HTLB_PGFAIL and VIRTIO_BALLOON_S_CACHES). > > > > > > > > I don't quite remember what we did here in the past in those cases, > > > > conditionally redefine the symbols in a local header, or protect the > > > > new code with an #ifdef? > > > > > > For what it's worth, my opinion is that the sensible options are to: > > > 1. Build against the latest stable, or a specified version of, kernel > > > headers; or 2. Protect with ifdef'ery until new definitions are > > > considered "common enough". > > > > > > Supporting older headers by grafting or even modifying required newer > > > definitions on top seems a horrid middle ground, albeit I can > > > appreciate the pragmatism of it. > > > > Fair enough, although I don't think option 1) is really viable for users, > > as upgrading the distro provided kernel headers is often not an option for > > the casual user. And even more versed users would probably shy away from > > staining their /usr/include directory just for kvmtool. > > > > Which just leaves option 2? If no one hollers, I will send a patch to that > > regard. > > How about copying the required headers to kvmtool, under include/linux? > That would remove any dependency on a specific kernel or distro version. Maintaining just the required headers sounds a bit of a pain. Getting it wrong ends up copying too many headers (and there's nearly 200kLOC of them) or a confusing split between copied and system-installed headers. How about requiring headers at include/linux and if the required version tag isn't found there, download the kernel tree and "make headers_install" with customised INSTALL_HDR_PATH? The cost is a big(ish) download: time, bandwidth, disk space. -- Keir > Thanks, > Alex > > > > > Cheers, > > Andre > > > > > > > > > > Regards, > > > Keir > > > > > > > > > > I would lean towards the former (and hacking this in works), but then we > > > > would need to redefine VIRTIO_BALLOON_S_NR, to encompass the new symbols, > > > > which sounds fragile. > > > > > > > > Happy to send a patch if we agree on an approach. > > > > > > > > Cheers, > > > > Andre > > > > > > > > > > > > > > > > Keir Fraser (2): > > > > > > virtio/balloon: Fix a crash when collecting stats > > > > > > stat: Add descriptions for new virtio_balloon stat types > > > > > > > > > > > > [...] > > > > > > > > > > Applied to kvmtool (master), thanks! > > > > > > > > > > [1/2] virtio/balloon: Fix a crash when collecting stats > > > > > https://git.kernel.org/will/kvmtool/c/3a13530ae99a > > > > > [2/2] stat: Add descriptions for new virtio_balloon stat types > > > > > https://git.kernel.org/will/kvmtool/c/bc77bf49df6e > > > > > > > > > > Cheers, > > > > > >
