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

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

[Keir Fraser]

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.

Keir Fraser (2):
  virtio/balloon: Fix a crash when collecting stats
  stat: Add descriptions for new virtio_balloon stat types

 builtin-stat.c   | 17 ++++++++++++++++-
 virtio/balloon.c |  7 ++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog

[PATCH kvmtool 1/2] virtio/balloon: Fix a crash when collecting stats

[Keir Fraser]

The collect_stats hook dereferences the stats virtio queue without
checking that it has been initialised.

Signed-off-by: Keir Fraser <keirf@google.com>
Cc: Will Deacon <will@kernel.org>
---
 virtio/balloon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/virtio/balloon.c b/virtio/balloon.c
index 8e8803f..7c7b115 100644
--- a/virtio/balloon.c
+++ b/virtio/balloon.c
@@ -126,9 +126,14 @@ static void virtio_bln_do_io(struct kvm *kvm, void *param)
 
 static int virtio_bln__collect_stats(struct kvm *kvm)
 {
+	struct virt_queue *vq = &bdev.vqs[VIRTIO_BLN_STATS];
 	u64 tmp;
 
-	virt_queue__set_used_elem(&bdev.vqs[VIRTIO_BLN_STATS], bdev.cur_stat_head,
+	/* Exit if the queue is not set up. */
+	if (!vq->pfn)
+		return -ENODEV;
+
+	virt_queue__set_used_elem(vq, bdev.cur_stat_head,
 				  sizeof(struct virtio_balloon_stat));
 	bdev.vdev.ops->signal_vq(kvm, &bdev.vdev, VIRTIO_BLN_STATS);
 
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH kvmtool 2/2] stat: Add descriptions for new virtio_balloon stat types

[Keir Fraser]

Unknown types would print the value with no descriptive text at all.
Add descriptions for all known stat types, and a default description
when the type is unknown.

Signed-off-by: Keir Fraser <keirf@google.com>
Cc: Will Deacon <will@kernel.org>
---
 builtin-stat.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin-stat.c b/builtin-stat.c
index 5d6407e..876e96a 100644
--- a/builtin-stat.c
+++ b/builtin-stat.c
@@ -83,11 +83,26 @@ static int do_memstat(const char *name, int sock)
 		case VIRTIO_BALLOON_S_MINFLT:
 			printf("The number of minor page faults that have occurred:");
 			break;
+		case VIRTIO_BALLOON_S_HTLB_PGALLOC:
+			printf("The number of successful HugeTLB allocations:");
+			break;
+		case VIRTIO_BALLOON_S_HTLB_PGFAIL:
+			printf("The number of failed HugeTLB allocations:");
+			break;
 		case VIRTIO_BALLOON_S_MEMFREE:
 			printf("The amount of memory not being used for any purpose (in bytes):");
 			break;
 		case VIRTIO_BALLOON_S_MEMTOT:
-			printf("The total amount of memory available (in bytes):");
+			printf("The total amount of memory (in bytes):");
+			break;
+		case VIRTIO_BALLOON_S_AVAIL:
+			printf("The estimated available memory (in bytes):");
+			break;
+		case VIRTIO_BALLOON_S_CACHES:
+			printf("The amount of memory in use for file caching (in bytes):");
+			break;
+		default:
+			printf("Unknown memory statistic (ID %u): ", stats[i].tag);
 			break;
 		}
 		printf("%llu\n", (unsigned long long)stats[i].val);
-- 
2.36.1.124.g0e6072fb45-goog

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

[Will Deacon]

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

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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

[Andre Przywara]

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?

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,

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

[Keir Fraser]

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.

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

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

[Andre Przywara]

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

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

[Alexandru Elisei]

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

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

[Keir Fraser]

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

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

[Alexandru Elisei]

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 wrong ends up copying too many headers (and there's nearly 200kLOC

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.

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

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

[Andre Przywara]

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

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

[Alexandru Elisei]

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

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