Re: [PATCH kvmtool 0/2] Fixes for virtio_balloon stats printing

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 23 May 2022 15:06:23 +0000
Keir Fraser <keirf@google.com> 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@kernel.org> 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.

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