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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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

How about copying the required headers to kvmtool, under include/linux?
That would remove any dependency on a specific kernel or distro version.

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