Hi, On Mon, May 23, 2022 at 06:39:32PM +0100, Andre Przywara wrote: > On Mon, 23 May 2022 18:07:41 +0100 > Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote: > > > Hi, > > > > On Mon, May 23, 2022 at 04:35:30PM +0000, Keir Fraser wrote: > > > 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 > > > > Isn't the Linux mantra "don't break userspace"? So in that case, even if > > kvmtool uses an older version of a header, that won't cause any issues, > > right? > > It should work in either way, we just might end up ignoring new features, > if we use older header files. I don't think that's a problem, whenever kvmtool gets support for a feature not in the existing headers, the headers can be updated, similar to the other KVM headers. > > > > it wrong ends up copying too many headers (and there's nearly 200kLOC > > I feel we shouldn't go crazy here just because of some statistic feature. > kvmtool is meant to be a lean and mean tool, compiling cleanly on as many > systems as possible, as a pure user. So mandating header updates from > innocent users sounds a bit over the top. > > > We could mandate that the kernel header file is copied only when new > > features are added to kvmtool. I don't think there's any need to do it > > retroactively. > > Just thinking: we do the header sync already for the KVM related > headers, as we need them, and didn't want to wait for distros. > Can't we just include some virtio headers in that list as well? I like this idea. Thanks, Alex > > Cheers, > Andre > > > > > What do you think? > > > > Thanks, > > Alex > > > > > 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, > > > > > > > > > > > > >
