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

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

On Mon, 23 May 2022 18:07:41 +0100
Alexandru Elisei <alexandru.elisei@arm.com> 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@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.  
> > 
> > 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.

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

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