[PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Will Deacon]

Hello folks,

Here's the latest round of ARM fixes and updates for kvmtool. Most of
this is confined to the arm/ subdirectory, with the exception of a fix
to the virtio-mmio vq definitions due to the multi-queue work from
Sasha. I'm not terribly happy about that code though, since it seriously
increases the memory footprint of the guest.

Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
the new changes, that increases to 170MB! Any chance we can try and tackle
this regression please? I keep getting bitten by the OOM killer :(

Will

Marc Zyngier (4):
  kvm tools: arm: don't crash when no compatible CPU is found
  kvm tools: arm: add CPU compatible string to target structure
  kvm tools: arm: consolidate CPU node generation
  kvm tools: arm64: add support for AEM and Foundation models

Will Deacon (1):
  kvm tools: bump number of virtio MMIO vqueues

 tools/kvm/arm/aarch32/cortex-a15.c              | 39 ++--------------
 tools/kvm/arm/aarch64/cortex-a57.c              | 59 +++++++++----------------
 tools/kvm/arm/fdt.c                             | 35 +++++++++++++--
 tools/kvm/arm/include/arm-common/kvm-cpu-arch.h |  6 ++-
 tools/kvm/arm/kvm-cpu.c                         | 11 +++--
 tools/kvm/include/kvm/virtio-mmio.h             |  2 +-
 6 files changed, 70 insertions(+), 82 deletions(-)

-- 
1.8.0

[PATCH 1/5] kvm tools: arm: don't crash when no compatible CPU is found

[Will Deacon]

From: Marc Zyngier <Marc.Zyngier@arm.com>

If the kernel against which kvm tools was compiled supports more
CPU types than kvm tools does, then we can hit a situation where
we dereference an empty target slot.

Just stepping over empty slots fixes the issue.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/kvm/arm/kvm-cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
index 7a0eff45..2716690 100644
--- a/tools/kvm/arm/kvm-cpu.c
+++ b/tools/kvm/arm/kvm-cpu.c
@@ -56,6 +56,8 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 
 	/* Find an appropriate target CPU type. */
 	for (i = 0; i < ARRAY_SIZE(kvm_arm_targets); ++i) {
+		if (!kvm_arm_targets[i])
+			continue;
 		vcpu_init.target = kvm_arm_targets[i]->id;
 		err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
 		if (!err)
-- 
1.8.0

[PATCH 2/5] kvm tools: arm: add CPU compatible string to target structure

[Will Deacon]

From: Marc Zyngier <Marc.Zyngier@arm.com>

Instead of hardcoding the CPU compatible string, store it in
the target descriptor. This allows similar CPUs to be managed
by the same backend, and yet have different compatible
properties.

Also remove a check for a condition that can never occur in both
A15 and A57 backends.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/kvm/arm/aarch32/cortex-a15.c              | 12 ++++--------
 tools/kvm/arm/aarch64/cortex-a57.c              | 12 ++++--------
 tools/kvm/arm/include/arm-common/kvm-cpu-arch.h |  6 ++++--
 tools/kvm/arm/kvm-cpu.c                         |  9 ++++++---
 4 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/tools/kvm/arm/aarch32/cortex-a15.c b/tools/kvm/arm/aarch32/cortex-a15.c
index 8031747..4030e53 100644
--- a/tools/kvm/arm/aarch32/cortex-a15.c
+++ b/tools/kvm/arm/aarch32/cortex-a15.c
@@ -20,16 +20,11 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
 	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
 		char cpu_name[CPU_NAME_MAX_LEN];
 
-		if (kvm->cpus[cpu]->cpu_type != KVM_ARM_TARGET_CORTEX_A15) {
-			pr_warning("Ignoring unknown type for CPU %d\n", cpu);
-			continue;
-		}
-
 		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%d", cpu);
 
 		_FDT(fdt_begin_node(fdt, cpu_name));
 		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
-		_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15"));
+		_FDT(fdt_property_string(fdt, "compatible", kvm->cpus[cpu]->cpu_compatible));
 
 		if (kvm->nrcpus > 1)
 			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
@@ -83,8 +78,9 @@ static int cortex_a15__vcpu_init(struct kvm_cpu *vcpu)
 }
 
 static struct kvm_arm_target target_cortex_a15 = {
-	.id	= KVM_ARM_TARGET_CORTEX_A15,
-	.init	= cortex_a15__vcpu_init,
+	.id		= KVM_ARM_TARGET_CORTEX_A15,
+	.compatible	= "arm,cortex-a15",
+	.init		= cortex_a15__vcpu_init,
 };
 
 static int cortex_a15__core_init(struct kvm *kvm)
diff --git a/tools/kvm/arm/aarch64/cortex-a57.c b/tools/kvm/arm/aarch64/cortex-a57.c
index 4fd11ba..f636ef7 100644
--- a/tools/kvm/arm/aarch64/cortex-a57.c
+++ b/tools/kvm/arm/aarch64/cortex-a57.c
@@ -20,16 +20,11 @@ static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
 	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
 		char cpu_name[CPU_NAME_MAX_LEN];
 
-		if (kvm->cpus[cpu]->cpu_type != KVM_ARM_TARGET_CORTEX_A57) {
-			pr_warning("Ignoring unknown type for CPU %d\n", cpu);
-			continue;
-		}
-
 		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%d", cpu);
 
 		_FDT(fdt_begin_node(fdt, cpu_name));
 		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
-		_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a57"));
+		_FDT(fdt_property_string(fdt, "compatible", kvm->cpus[cpu]->cpu_compatible));
 
 		if (kvm->nrcpus > 1)
 			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
@@ -84,8 +79,9 @@ static int cortex_a57__vcpu_init(struct kvm_cpu *vcpu)
 }
 
 static struct kvm_arm_target target_cortex_a57 = {
-	.id	= KVM_ARM_TARGET_CORTEX_A57,
-	.init	= cortex_a57__vcpu_init,
+	.id		= KVM_ARM_TARGET_CORTEX_A57,
+	.compatible	= "arm,cortex-a57",
+	.init		= cortex_a57__vcpu_init,
 };
 
 static int cortex_a57__core_init(struct kvm *kvm)
diff --git a/tools/kvm/arm/include/arm-common/kvm-cpu-arch.h b/tools/kvm/arm/include/arm-common/kvm-cpu-arch.h
index 351fbe6..b514dd5 100644
--- a/tools/kvm/arm/include/arm-common/kvm-cpu-arch.h
+++ b/tools/kvm/arm/include/arm-common/kvm-cpu-arch.h
@@ -12,6 +12,7 @@ struct kvm_cpu {
 
 	unsigned long	cpu_id;
 	unsigned long	cpu_type;
+	const char	*cpu_compatible;
 
 	struct kvm	*kvm;
 	int		vcpu_fd;
@@ -28,8 +29,9 @@ struct kvm_cpu {
 };
 
 struct kvm_arm_target {
-	u32	id;
-	int	(*init)(struct kvm_cpu *vcpu);
+	u32		id;
+	const char 	*compatible;
+	int		(*init)(struct kvm_cpu *vcpu);
 };
 
 int kvm_cpu__register_kvm_arm_target(struct kvm_arm_target *target);
diff --git a/tools/kvm/arm/kvm-cpu.c b/tools/kvm/arm/kvm-cpu.c
index 2716690..6d4f306 100644
--- a/tools/kvm/arm/kvm-cpu.c
+++ b/tools/kvm/arm/kvm-cpu.c
@@ -30,6 +30,7 @@ int kvm_cpu__register_kvm_arm_target(struct kvm_arm_target *target)
 
 struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 {
+	struct kvm_arm_target *target;
 	struct kvm_cpu *vcpu;
 	int coalesced_offset, mmap_size, err = -1;
 	unsigned int i;
@@ -58,13 +59,14 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 	for (i = 0; i < ARRAY_SIZE(kvm_arm_targets); ++i) {
 		if (!kvm_arm_targets[i])
 			continue;
-		vcpu_init.target = kvm_arm_targets[i]->id;
+		target = kvm_arm_targets[i];
+		vcpu_init.target = target->id;
 		err = ioctl(vcpu->vcpu_fd, KVM_ARM_VCPU_INIT, &vcpu_init);
 		if (!err)
 			break;
 	}
 
-	if (err || kvm_arm_targets[i]->init(vcpu))
+	if (err || target->init(vcpu))
 		die("Unable to initialise ARM vcpu");
 
 	coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION,
@@ -76,7 +78,8 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 	/* Populate the vcpu structure. */
 	vcpu->kvm		= kvm;
 	vcpu->cpu_id		= cpu_id;
-	vcpu->cpu_type		= vcpu_init.target;
+	vcpu->cpu_type		= target->id;
+	vcpu->cpu_compatible	= target->compatible;
 	vcpu->is_running	= true;
 	return vcpu;
 }
-- 
1.8.0

[PATCH 3/5] kvm tools: arm: consolidate CPU node generation

[Will Deacon]

From: Marc Zyngier <Marc.Zyngier@arm.com>

Now that generate_cpu_nodes uses the cpu_compatible field to
output the compatible property, we can unify the A15 and A57
implementations, as they are strictly identical.

Move the function to fdt.c, together with most of the device
tree generation.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/kvm/arm/aarch32/cortex-a15.c | 29 -----------------------------
 tools/kvm/arm/aarch64/cortex-a57.c | 29 -----------------------------
 tools/kvm/arm/fdt.c                | 35 ++++++++++++++++++++++++++++++++---
 3 files changed, 32 insertions(+), 61 deletions(-)

diff --git a/tools/kvm/arm/aarch32/cortex-a15.c b/tools/kvm/arm/aarch32/cortex-a15.c
index 4030e53..ca65af7 100644
--- a/tools/kvm/arm/aarch32/cortex-a15.c
+++ b/tools/kvm/arm/aarch32/cortex-a15.c
@@ -8,34 +8,6 @@
 #include <linux/byteorder.h>
 #include <linux/types.h>
 
-#define CPU_NAME_MAX_LEN 8
-static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
-{
-	int cpu;
-
-	_FDT(fdt_begin_node(fdt, "cpus"));
-	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
-	_FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
-
-	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
-		char cpu_name[CPU_NAME_MAX_LEN];
-
-		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%d", cpu);
-
-		_FDT(fdt_begin_node(fdt, cpu_name));
-		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
-		_FDT(fdt_property_string(fdt, "compatible", kvm->cpus[cpu]->cpu_compatible));
-
-		if (kvm->nrcpus > 1)
-			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
-
-		_FDT(fdt_property_cell(fdt, "reg", cpu));
-		_FDT(fdt_end_node(fdt));
-	}
-
-	_FDT(fdt_end_node(fdt));
-}
-
 static void generate_timer_nodes(void *fdt, struct kvm *kvm)
 {
 	u32 cpu_mask = (((1 << kvm->nrcpus) - 1) << GIC_FDT_IRQ_PPI_CPU_SHIFT) \
@@ -66,7 +38,6 @@ static void generate_timer_nodes(void *fdt, struct kvm *kvm)
 
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
-	generate_cpu_nodes(fdt, kvm);
 	gic__generate_fdt_nodes(fdt, gic_phandle);
 	generate_timer_nodes(fdt, kvm);
 }
diff --git a/tools/kvm/arm/aarch64/cortex-a57.c b/tools/kvm/arm/aarch64/cortex-a57.c
index f636ef7..5b0dc4c 100644
--- a/tools/kvm/arm/aarch64/cortex-a57.c
+++ b/tools/kvm/arm/aarch64/cortex-a57.c
@@ -8,34 +8,6 @@
 #include <linux/byteorder.h>
 #include <linux/types.h>
 
-#define CPU_NAME_MAX_LEN 8
-static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
-{
-	int cpu;
-
-	_FDT(fdt_begin_node(fdt, "cpus"));
-	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
-	_FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
-
-	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
-		char cpu_name[CPU_NAME_MAX_LEN];
-
-		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%d", cpu);
-
-		_FDT(fdt_begin_node(fdt, cpu_name));
-		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
-		_FDT(fdt_property_string(fdt, "compatible", kvm->cpus[cpu]->cpu_compatible));
-
-		if (kvm->nrcpus > 1)
-			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
-
-		_FDT(fdt_property_cell(fdt, "reg", cpu));
-		_FDT(fdt_end_node(fdt));
-	}
-
-	_FDT(fdt_end_node(fdt));
-}
-
 static void generate_timer_nodes(void *fdt, struct kvm *kvm)
 {
 	u32 cpu_mask = (((1 << kvm->nrcpus) - 1) << GIC_FDT_IRQ_PPI_CPU_SHIFT) \
@@ -66,7 +38,6 @@ static void generate_timer_nodes(void *fdt, struct kvm *kvm)
 
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
-	generate_cpu_nodes(fdt, kvm);
 	gic__generate_fdt_nodes(fdt, gic_phandle);
 	generate_timer_nodes(fdt, kvm);
 }
diff --git a/tools/kvm/arm/fdt.c b/tools/kvm/arm/fdt.c
index 20e0308..c61bf58 100644
--- a/tools/kvm/arm/fdt.c
+++ b/tools/kvm/arm/fdt.c
@@ -41,6 +41,34 @@ static void dump_fdt(const char *dtb_file, void *fdt)
 	close(fd);
 }
 
+#define CPU_NAME_MAX_LEN 8
+static void generate_cpu_nodes(void *fdt, struct kvm *kvm)
+{
+	int cpu;
+
+	_FDT(fdt_begin_node(fdt, "cpus"));
+	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
+	_FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
+
+	for (cpu = 0; cpu < kvm->nrcpus; ++cpu) {
+		char cpu_name[CPU_NAME_MAX_LEN];
+
+		snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%d", cpu);
+
+		_FDT(fdt_begin_node(fdt, cpu_name));
+		_FDT(fdt_property_string(fdt, "device_type", "cpu"));
+		_FDT(fdt_property_string(fdt, "compatible", kvm->cpus[cpu]->cpu_compatible));
+
+		if (kvm->nrcpus > 1)
+			_FDT(fdt_property_string(fdt, "enable-method", "psci"));
+
+		_FDT(fdt_property_cell(fdt, "reg", cpu));
+		_FDT(fdt_end_node(fdt));
+	}
+
+	_FDT(fdt_end_node(fdt));
+}
+
 #define DEVICE_NAME_MAX_LEN 32
 static void generate_virtio_mmio_node(void *fdt, struct virtio_mmio *vmmio)
 {
@@ -77,7 +105,7 @@ static int setup_fdt(struct kvm *kvm)
 	void *fdt		= staging_fdt;
 	void *fdt_dest		= guest_flat_to_host(kvm,
 						     kvm->arch.dtb_guest_start);
-	void (*generate_cpu_nodes)(void *, struct kvm *, u32)
+	void (*generate_fdt_nodes)(void *, struct kvm *, u32)
 				= kvm->cpus[0]->generate_fdt_nodes;
 
 	/* Create new tree without a reserve map */
@@ -115,8 +143,9 @@ static int setup_fdt(struct kvm *kvm)
 	_FDT(fdt_end_node(fdt));
 
 	/* CPU and peripherals (interrupt controller, timers, etc) */
-	if (generate_cpu_nodes)
-		generate_cpu_nodes(fdt, kvm, gic_phandle);
+	generate_cpu_nodes(fdt, kvm);
+	if (generate_fdt_nodes)
+		generate_fdt_nodes(fdt, kvm, gic_phandle);
 
 	/* Virtio MMIO devices */
 	dev_hdr = device__first_dev(DEVICE_BUS_MMIO);
-- 
1.8.0

[PATCH 4/5] kvm tools: arm64: add support for AEM and Foundation models

[Will Deacon]

From: Marc Zyngier <Marc.Zyngier@arm.com>

The ARMv8 architecture is supported by two publicly available
software models: the Architecture Enveloppe Model, and the
Foundation model.

Both are fairly similar to the Cortex-A57 from a kvm tools point of
view, so we can hijack the A57 implementation to register these
new targets.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 tools/kvm/arm/aarch64/cortex-a57.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/arm/aarch64/cortex-a57.c b/tools/kvm/arm/aarch64/cortex-a57.c
index 5b0dc4c..0c340fb 100644
--- a/tools/kvm/arm/aarch64/cortex-a57.c
+++ b/tools/kvm/arm/aarch64/cortex-a57.c
@@ -49,6 +49,22 @@ static int cortex_a57__vcpu_init(struct kvm_cpu *vcpu)
 	return 0;
 }
 
+/*
+ * As far as userspace is concerned, both of these implementations are
+ * extremely similar.
+ */
+static struct kvm_arm_target target_aem_v8 = {
+	.id		= KVM_ARM_TARGET_AEM_V8,
+	.compatible	= "arm,arm-v8",
+	.init		= cortex_a57__vcpu_init,
+};
+
+static struct kvm_arm_target target_foundation_v8 = {
+	.id		= KVM_ARM_TARGET_FOUNDATION_V8,
+	.compatible	= "arm,arm-v8",
+	.init		= cortex_a57__vcpu_init,
+};
+
 static struct kvm_arm_target target_cortex_a57 = {
 	.id		= KVM_ARM_TARGET_CORTEX_A57,
 	.compatible	= "arm,cortex-a57",
@@ -57,6 +73,8 @@ static struct kvm_arm_target target_cortex_a57 = {
 
 static int cortex_a57__core_init(struct kvm *kvm)
 {
-	return kvm_cpu__register_kvm_arm_target(&target_cortex_a57);
+	return (kvm_cpu__register_kvm_arm_target(&target_aem_v8) ||
+		kvm_cpu__register_kvm_arm_target(&target_foundation_v8) ||
+		kvm_cpu__register_kvm_arm_target(&target_cortex_a57));
 }
 core_init(cortex_a57__core_init);
-- 
1.8.0

[PATCH 5/5] kvm tools: bump number of virtio MMIO vqueues

[Will Deacon]

Commit 4d789d4a2050 ("kvm tools: Increase amount of possible interrupts
per PCI device") increased the maximum amount of virtio queues for the
PCI transport, but neglected to do the same for MMIO.

This patch makes the same change for virtio-mmio.

Cc: Sasha Levin <sasha.levin@oracle.com>
Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

Sasha -- although this fixes a SEGV when booting a guest which tries to
use virtio-net over MMIO, it *drastically* increases the memory footprint
of a guest kernel to unacceptable levels (38MB -> 170MB).

 tools/kvm/include/kvm/virtio-mmio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/kvm/include/kvm/virtio-mmio.h b/tools/kvm/include/kvm/virtio-mmio.h
index 983c8fc..4d6a671 100644
--- a/tools/kvm/include/kvm/virtio-mmio.h
+++ b/tools/kvm/include/kvm/virtio-mmio.h
@@ -4,7 +4,7 @@
 #include <linux/types.h>
 #include <linux/virtio_mmio.h>
 
-#define VIRTIO_MMIO_MAX_VQ	3
+#define VIRTIO_MMIO_MAX_VQ	32
 #define VIRTIO_MMIO_MAX_CONFIG	1
 #define VIRTIO_MMIO_IO_SIZE	0x200
 
-- 
1.8.0

Re: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Sasha Levin]

On 04/11/2013 12:36 PM, Will Deacon wrote:
> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
> the new changes, that increases to 170MB! Any chance we can try and tackle
> this regression please? I keep getting bitten by the OOM killer :(

That's definitely unwanted.

I'll look into it and try sending something out today/tomorrow.


Thanks,
Sasha

virtio-net mq vq initialization (was: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool)

[Sasha Levin]

On 04/11/2013 12:36 PM, Will Deacon wrote:
> Hello folks,
> 
> Here's the latest round of ARM fixes and updates for kvmtool. Most of
> this is confined to the arm/ subdirectory, with the exception of a fix
> to the virtio-mmio vq definitions due to the multi-queue work from
> Sasha. I'm not terribly happy about that code though, since it seriously
> increases the memory footprint of the guest.
> 
> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
> the new changes, that increases to 170MB! Any chance we can try and tackle
> this regression please? I keep getting bitten by the OOM killer :(

(cc Rusty, MST)

The spec defines the operation of a virtio-net device with regards to multiple
queues as follows:

"""
Device Initialization

	1. The initialization routine should identify the receive and transmission
virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.

	[...]

	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
up to max_virtqueue_pairs of each of transmit and receive queues; execute_
VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
the number of the transmit and receive queues that is going to be
used and wait until the device consumes the controlq buffer and acks this
command.
"""

And kvmtool follows that to the letter: It will initialize the maximum amount of
queues it can support during initialization and will start using them only when
the device tells it it should use them.

As Will has stated, this causes a memory issue since all the data structures that hold
all possible queues get initialized regardless of whether we actually need them or not,
which is quite troublesome for systems with small RAM.


Rusty, MST, would you be open to a spec and code change that would initialize the
RX/TX vqs on demand instead of on device initialization? Or is there an easier way
to work around this issue?


Thanks,
Sasha

Re: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Pekka Enberg]

On 04/11/2013 12:36 PM, Will Deacon wrote:
>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>> the new changes, that increases to 170MB! Any chance we can try and tackle
>> this regression please? I keep getting bitten by the OOM killer :(

On 04/11/2013 07:45 PM, Sasha Levin wrote:
> That's definitely unwanted.
>
> I'll look into it and try sending something out today/tomorrow.

That's very unfortunate. Can you please confirm that reverting commit 
e026314820acc3cc967308355f3746aca238fda4 ("kvm tools: virtio-net 
multiqueue support") fixes it? If fixing the issue turns out to be 
difficult, we should just take multi-queue out until the issues is resolved.

			Pekka

Re: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Pekka Enberg]

On 04/11/2013 07:36 PM, Will Deacon wrote:
> Here's the latest round of ARM fixes and updates for kvmtool.

Applied, thanks a lot Will!

			Pekka

Re: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Marc Zyngier]

Hi Pekka,

On 12/04/13 07:52, Pekka Enberg wrote:
> On 04/11/2013 12:36 PM, Will Deacon wrote:
>>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>>> the new changes, that increases to 170MB! Any chance we can try and tackle
>>> this regression please? I keep getting bitten by the OOM killer :(
> 
> On 04/11/2013 07:45 PM, Sasha Levin wrote:
>> That's definitely unwanted.
>>
>> I'll look into it and try sending something out today/tomorrow.
> 
> That's very unfortunate. Can you please confirm that reverting commit 
> e026314820acc3cc967308355f3746aca238fda4 ("kvm tools: virtio-net 
> multiqueue support") fixes it? If fixing the issue turns out to be 
> difficult, we should just take multi-queue out until the issues is resolved.

It does, at least for an MMIO based setup.

It would be a pity to completely loose the functionality though. Surely
we can find a way to initialize the extra queues on demand. And if not,
maybe introduce some kind of tunable parameter...

Cheers,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool

[Will Deacon]

On Fri, Apr 12, 2013 at 09:30:18AM +0100, Marc Zyngier wrote:
> Hi Pekka,
> 
> On 12/04/13 07:52, Pekka Enberg wrote:
> > On 04/11/2013 12:36 PM, Will Deacon wrote:
> >>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
> >>> the new changes, that increases to 170MB! Any chance we can try and tackle
> >>> this regression please? I keep getting bitten by the OOM killer :(
> > 
> > On 04/11/2013 07:45 PM, Sasha Levin wrote:
> >> That's definitely unwanted.
> >>
> >> I'll look into it and try sending something out today/tomorrow.
> > 
> > That's very unfortunate. Can you please confirm that reverting commit 
> > e026314820acc3cc967308355f3746aca238fda4 ("kvm tools: virtio-net 
> > multiqueue support") fixes it? If fixing the issue turns out to be 
> > difficult, we should just take multi-queue out until the issues is resolved.
> 
> It does, at least for an MMIO based setup.
> 
> It would be a pity to completely loose the functionality though. Surely
> we can find a way to initialize the extra queues on demand. And if not,
> maybe introduce some kind of tunable parameter...

I suppose we *could* move the whole thing over to dynamic allocation and
let the maximum number of queues be an lkvm parameter, but it feels like a
lot of work without really solving the problem.

Anyway, thanks Sasha for starting a separate thread about this. Hopefully we
can resolve it in the spec and then update kvmtool/linux accordingly. As
long as the issue is being addressed, then I don't see that there's a
pressing need to revert the commit above.

Cheers,

Will

Re: virtio-net mq vq initialization (was: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool)

[Rusty Russell]

Sasha Levin <sasha.levin@oracle.com> writes:
> On 04/11/2013 12:36 PM, Will Deacon wrote:
>> Hello folks,
>> 
>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
>> this is confined to the arm/ subdirectory, with the exception of a fix
>> to the virtio-mmio vq definitions due to the multi-queue work from
>> Sasha. I'm not terribly happy about that code though, since it seriously
>> increases the memory footprint of the guest.
>> 
>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>> the new changes, that increases to 170MB! Any chance we can try and tackle
>> this regression please? I keep getting bitten by the OOM killer :(
>
> (cc Rusty, MST)
>
> The spec defines the operation of a virtio-net device with regards to multiple
> queues as follows:
>
> """
> Device Initialization
>
> 	1. The initialization routine should identify the receive and transmission
> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
>
> 	[...]
>
> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
> the number of the transmit and receive queues that is going to be
> used and wait until the device consumes the controlq buffer and acks this
> command.
> """
>
> And kvmtool follows that to the letter: It will initialize the maximum amount of
> queues it can support during initialization and will start using them only when
> the device tells it it should use them.
>
> As Will has stated, this causes a memory issue since all the data structures that hold
> all possible queues get initialized regardless of whether we actually need them or not,
> which is quite troublesome for systems with small RAM.
>
>
> Rusty, MST, would you be open to a spec and code change that would initialize the
> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
> to work around this issue?

I'm confused.  kvmtool is using too much memory, or the guest?  If
kvmtool, the Device Initialization section above applies to the driver,
not the device.  If the guest, well, the language says "UP TO N+1".  You
want a small guest, don't use them all.  Or any...

What am I missing?
Rusty.

Re: virtio-net mq vq initialization (was: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool)

[Will Deacon]

Hi Rusty,

On Fri, Apr 12, 2013 at 12:36:00PM +0100, Rusty Russell wrote:
> I'm confused.  kvmtool is using too much memory, or the guest?  If
> kvmtool, the Device Initialization section above applies to the driver,
> not the device.  If the guest, well, the language says "UP TO N+1".  You
> want a small guest, don't use them all.  Or any...
> 
> What am I missing?

I don't think you're missing anything. The memory usage is in the guest, not
kvmtool. If, as you suggest, the spec doesn't mandate greedy initialisation
of all the virtqueues, then it sounds like a bug in Linux.

Looking at virtio_mmio.c:

        /* Allocate pages for the queue - start with a queue as big as
         * possible (limited by maximum size allowed by device), drop down
         * to a minimal size, just big enough to fit descriptor table
         * and two rings (which makes it "alignment_size * 2")
         */
        info->num = readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);

        /* If the device reports a 0 entry queue, we won't be able to
         * use it to perform I/O, and vring_new_virtqueue() can't create
         * empty queues anyway, so don't bother to set up the device.
         */
        if (info->num == 0) {
                err = -ENOENT;
                goto error_alloc_pages;
        }

        while (1) {
                size = PAGE_ALIGN(vring_size(info->num,
                                VIRTIO_MMIO_VRING_ALIGN));
                /* Did the last iter shrink the queue below minimum size? */
                if (size < VIRTIO_MMIO_VRING_ALIGN * 2) {
                        err = -ENOMEM;
                        goto error_alloc_pages;
                }

                info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
                if (info->queue)
                        break;

                info->num /= 2;
        }

so, although there is some back-off if the allocation fails, it doesn't really
help userspace when there is enough memory for the maximal buffers, but not
a lot else.

Will

Re: virtio-net mq vq initialization

[Sasha Levin]

On 04/12/2013 07:36 AM, Rusty Russell wrote:
> Sasha Levin <sasha.levin@oracle.com> writes:
>> On 04/11/2013 12:36 PM, Will Deacon wrote:
>>> Hello folks,
>>>
>>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
>>> this is confined to the arm/ subdirectory, with the exception of a fix
>>> to the virtio-mmio vq definitions due to the multi-queue work from
>>> Sasha. I'm not terribly happy about that code though, since it seriously
>>> increases the memory footprint of the guest.
>>>
>>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>>> the new changes, that increases to 170MB! Any chance we can try and tackle
>>> this regression please? I keep getting bitten by the OOM killer :(
>>
>> (cc Rusty, MST)
>>
>> The spec defines the operation of a virtio-net device with regards to multiple
>> queues as follows:
>>
>> """
>> Device Initialization
>>
>> 	1. The initialization routine should identify the receive and transmission
>> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
>> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
>>
>> 	[...]
>>
>> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
>> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
>> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
>> the number of the transmit and receive queues that is going to be
>> used and wait until the device consumes the controlq buffer and acks this
>> command.
>> """
>>
>> And kvmtool follows that to the letter: It will initialize the maximum amount of
>> queues it can support during initialization and will start using them only when
>> the device tells it it should use them.
>>
>> As Will has stated, this causes a memory issue since all the data structures that hold
>> all possible queues get initialized regardless of whether we actually need them or not,
>> which is quite troublesome for systems with small RAM.
>>
>>
>> Rusty, MST, would you be open to a spec and code change that would initialize the
>> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
>> to work around this issue?
> 
> I'm confused.  kvmtool is using too much memory, or the guest?  If
> kvmtool, the Device Initialization section above applies to the driver,
> not the device.  If the guest, well, the language says "UP TO N+1".  You
> want a small guest, don't use them all.  Or any...
> 
> What am I missing?

It's in the guest - sorry. I was only trying to say that kvmtool doesn't do anything
odd with regards to initializing virtio-net.

The thing is that there should be a difference between just allowing a larger number
of queues and actually using them (i.e. enabling them with ethtool). Right now I see
the kernel lose 130MB just by having kvmtool offer 8 queue pairs, without actually
using those queues.

Yes, we can make it configurable in kvmtool (and I will make it so so the arm folks
could continue working with tiny guests) but does it make sense that you have to do
this configuration in *2* places? First in the hypervisor and then inside the guest?

Memory usage should ideally depend on whether you are actually using multiple queues,
not on whether you just allow using those queues.


Thanks,
Sasha

Re: virtio-net mq vq initialization

[Michael S. Tsirkin]

On Sat, Apr 13, 2013 at 05:23:41PM -0400, Sasha Levin wrote:
> On 04/12/2013 07:36 AM, Rusty Russell wrote:
> > Sasha Levin <sasha.levin@oracle.com> writes:
> >> On 04/11/2013 12:36 PM, Will Deacon wrote:
> >>> Hello folks,
> >>>
> >>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
> >>> this is confined to the arm/ subdirectory, with the exception of a fix
> >>> to the virtio-mmio vq definitions due to the multi-queue work from
> >>> Sasha. I'm not terribly happy about that code though, since it seriously
> >>> increases the memory footprint of the guest.
> >>>
> >>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
> >>> the new changes, that increases to 170MB! Any chance we can try and tackle
> >>> this regression please? I keep getting bitten by the OOM killer :(
> >>
> >> (cc Rusty, MST)
> >>
> >> The spec defines the operation of a virtio-net device with regards to multiple
> >> queues as follows:
> >>
> >> """
> >> Device Initialization
> >>
> >> 	1. The initialization routine should identify the receive and transmission
> >> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
> >> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
> >>
> >> 	[...]
> >>
> >> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
> >> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
> >> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
> >> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
> >> the number of the transmit and receive queues that is going to be
> >> used and wait until the device consumes the controlq buffer and acks this
> >> command.
> >> """
> >>
> >> And kvmtool follows that to the letter: It will initialize the maximum amount of
> >> queues it can support during initialization and will start using them only when
> >> the device tells it it should use them.
> >>
> >> As Will has stated, this causes a memory issue since all the data structures that hold
> >> all possible queues get initialized regardless of whether we actually need them or not,
> >> which is quite troublesome for systems with small RAM.
> >>
> >>
> >> Rusty, MST, would you be open to a spec and code change that would initialize the
> >> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
> >> to work around this issue?
> > 
> > I'm confused.  kvmtool is using too much memory, or the guest?  If
> > kvmtool, the Device Initialization section above applies to the driver,
> > not the device.  If the guest, well, the language says "UP TO N+1".  You
> > want a small guest, don't use them all.  Or any...
> > 
> > What am I missing?
> 
> It's in the guest - sorry. I was only trying to say that kvmtool doesn't do anything
> odd with regards to initializing virtio-net.
> 
> The thing is that there should be a difference between just allowing a larger number
> of queues and actually using them (i.e. enabling them with ethtool). Right now I see
> the kernel lose 130MB just by having kvmtool offer 8 queue pairs, without actually
> using those queues.
> 
> Yes, we can make it configurable in kvmtool (and I will make it so so the arm folks
> could continue working with tiny guests) but does it make sense that you have to do
> this configuration in *2* places? First in the hypervisor and then inside the guest?
> 
> Memory usage should ideally depend on whether you are actually using multiple queues,
> not on whether you just allow using those queues.
> 
> 
> Thanks,
> Sasha

8 queues eat up 130MB?  Most of the memory is likely for the buffers?  I
think we could easily allocate these lazily as queues are enabled,
without protocol changes. It's harder to clean them as there's no way to
reset a specific queue, but maybe that' good enough for your purposes?

-- 
MST

Re: virtio-net mq vq initialization (was: [PATCH 0/5] Usual batch of random ARM fixes for kvmtool)

[Michael S. Tsirkin]

On Fri, Apr 12, 2013 at 01:41:58PM +0100, Will Deacon wrote:
> Hi Rusty,
> 
> On Fri, Apr 12, 2013 at 12:36:00PM +0100, Rusty Russell wrote:
> > I'm confused.  kvmtool is using too much memory, or the guest?  If
> > kvmtool, the Device Initialization section above applies to the driver,
> > not the device.  If the guest, well, the language says "UP TO N+1".  You
> > want a small guest, don't use them all.  Or any...
> > 
> > What am I missing?
> 
> I don't think you're missing anything. The memory usage is in the guest, not
> kvmtool. If, as you suggest, the spec doesn't mandate greedy initialisation
> of all the virtqueues, then it sounds like a bug in Linux.
> 
> Looking at virtio_mmio.c:
> 
>         /* Allocate pages for the queue - start with a queue as big as
>          * possible (limited by maximum size allowed by device), drop down
>          * to a minimal size, just big enough to fit descriptor table
>          * and two rings (which makes it "alignment_size * 2")
>          */
>         info->num = readl(vm_dev->base + VIRTIO_MMIO_QUEUE_NUM_MAX);
> 
>         /* If the device reports a 0 entry queue, we won't be able to
>          * use it to perform I/O, and vring_new_virtqueue() can't create
>          * empty queues anyway, so don't bother to set up the device.
>          */
>         if (info->num == 0) {
>                 err = -ENOENT;
>                 goto error_alloc_pages;
>         }
> 
>         while (1) {
>                 size = PAGE_ALIGN(vring_size(info->num,
>                                 VIRTIO_MMIO_VRING_ALIGN));
>                 /* Did the last iter shrink the queue below minimum size? */
>                 if (size < VIRTIO_MMIO_VRING_ALIGN * 2) {
>                         err = -ENOMEM;
>                         goto error_alloc_pages;
>                 }
> 
>                 info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
>                 if (info->queue)
>                         break;
> 
>                 info->num /= 2;
>         }
> 
> so, although there is some back-off if the allocation fails, it doesn't really
> help userspace when there is enough memory for the maximal buffers, but not
> a lot else.
> 
> Will

Well, that's just a couple of pages per virtqueue. How many of these do
you have?

-- 
MST

Re: virtio-net mq vq initialization

[Sasha Levin]

On 04/14/2013 06:01 AM, Michael S. Tsirkin wrote:
> On Sat, Apr 13, 2013 at 05:23:41PM -0400, Sasha Levin wrote:
>> On 04/12/2013 07:36 AM, Rusty Russell wrote:
>>> Sasha Levin <sasha.levin@oracle.com> writes:
>>>> On 04/11/2013 12:36 PM, Will Deacon wrote:
>>>>> Hello folks,
>>>>>
>>>>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
>>>>> this is confined to the arm/ subdirectory, with the exception of a fix
>>>>> to the virtio-mmio vq definitions due to the multi-queue work from
>>>>> Sasha. I'm not terribly happy about that code though, since it seriously
>>>>> increases the memory footprint of the guest.
>>>>>
>>>>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>>>>> the new changes, that increases to 170MB! Any chance we can try and tackle
>>>>> this regression please? I keep getting bitten by the OOM killer :(
>>>>
>>>> (cc Rusty, MST)
>>>>
>>>> The spec defines the operation of a virtio-net device with regards to multiple
>>>> queues as follows:
>>>>
>>>> """
>>>> Device Initialization
>>>>
>>>> 	1. The initialization routine should identify the receive and transmission
>>>> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
>>>> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
>>>>
>>>> 	[...]
>>>>
>>>> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
>>>> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
>>>> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
>>>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
>>>> the number of the transmit and receive queues that is going to be
>>>> used and wait until the device consumes the controlq buffer and acks this
>>>> command.
>>>> """
>>>>
>>>> And kvmtool follows that to the letter: It will initialize the maximum amount of
>>>> queues it can support during initialization and will start using them only when
>>>> the device tells it it should use them.
>>>>
>>>> As Will has stated, this causes a memory issue since all the data structures that hold
>>>> all possible queues get initialized regardless of whether we actually need them or not,
>>>> which is quite troublesome for systems with small RAM.
>>>>
>>>>
>>>> Rusty, MST, would you be open to a spec and code change that would initialize the
>>>> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
>>>> to work around this issue?
>>>
>>> I'm confused.  kvmtool is using too much memory, or the guest?  If
>>> kvmtool, the Device Initialization section above applies to the driver,
>>> not the device.  If the guest, well, the language says "UP TO N+1".  You
>>> want a small guest, don't use them all.  Or any...
>>>
>>> What am I missing?
>>
>> It's in the guest - sorry. I was only trying to say that kvmtool doesn't do anything
>> odd with regards to initializing virtio-net.
>>
>> The thing is that there should be a difference between just allowing a larger number
>> of queues and actually using them (i.e. enabling them with ethtool). Right now I see
>> the kernel lose 130MB just by having kvmtool offer 8 queue pairs, without actually
>> using those queues.
>>
>> Yes, we can make it configurable in kvmtool (and I will make it so so the arm folks
>> could continue working with tiny guests) but does it make sense that you have to do
>> this configuration in *2* places? First in the hypervisor and then inside the guest?
>>
>> Memory usage should ideally depend on whether you are actually using multiple queues,
>> not on whether you just allow using those queues.
>>
>>
>> Thanks,
>> Sasha
> 
> 8 queues eat up 130MB?  Most of the memory is likely for the buffers?  I
> think we could easily allocate these lazily as queues are enabled,
> without protocol changes. It's harder to clean them as there's no way to
> reset a specific queue, but maybe that' good enough for your purposes?
> 

Yup, this is how it looks in the guest right after booting:

Without virtio-net mq:

# free
             total       used       free     shared    buffers     cached
Mem:        918112     158052     760060          0          0       4308
-/+ buffers/cache:     153744     764368

With queue pairs = 8:

# free
             total       used       free     shared    buffers     cached
Mem:        918112     289168     628944          0          0       4244
-/+ buffers/cache:     284924     633188


Initializing them only when they're actually needed will do the trick here.

We could also expose the facility to delete a single vq, and add a note to
the spec saying that if the amount of actual vq pairs was reduced below what
it was before, the now inactive queues become invalid and would need to be
re-initialized. It's not pretty but it would let both device and driver free
up those vqs.


Thanks,
Sasha

Re: virtio-net mq vq initialization

[Michael S. Tsirkin]

On Sun, Apr 14, 2013 at 11:16:46AM -0400, Sasha Levin wrote:
> On 04/14/2013 06:01 AM, Michael S. Tsirkin wrote:
> > On Sat, Apr 13, 2013 at 05:23:41PM -0400, Sasha Levin wrote:
> >> On 04/12/2013 07:36 AM, Rusty Russell wrote:
> >>> Sasha Levin <sasha.levin@oracle.com> writes:
> >>>> On 04/11/2013 12:36 PM, Will Deacon wrote:
> >>>>> Hello folks,
> >>>>>
> >>>>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
> >>>>> this is confined to the arm/ subdirectory, with the exception of a fix
> >>>>> to the virtio-mmio vq definitions due to the multi-queue work from
> >>>>> Sasha. I'm not terribly happy about that code though, since it seriously
> >>>>> increases the memory footprint of the guest.
> >>>>>
> >>>>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
> >>>>> the new changes, that increases to 170MB! Any chance we can try and tackle
> >>>>> this regression please? I keep getting bitten by the OOM killer :(
> >>>>
> >>>> (cc Rusty, MST)
> >>>>
> >>>> The spec defines the operation of a virtio-net device with regards to multiple
> >>>> queues as follows:
> >>>>
> >>>> """
> >>>> Device Initialization
> >>>>
> >>>> 	1. The initialization routine should identify the receive and transmission
> >>>> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
> >>>> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
> >>>>
> >>>> 	[...]
> >>>>
> >>>> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
> >>>> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
> >>>> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
> >>>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
> >>>> the number of the transmit and receive queues that is going to be
> >>>> used and wait until the device consumes the controlq buffer and acks this
> >>>> command.
> >>>> """
> >>>>
> >>>> And kvmtool follows that to the letter: It will initialize the maximum amount of
> >>>> queues it can support during initialization and will start using them only when
> >>>> the device tells it it should use them.
> >>>>
> >>>> As Will has stated, this causes a memory issue since all the data structures that hold
> >>>> all possible queues get initialized regardless of whether we actually need them or not,
> >>>> which is quite troublesome for systems with small RAM.
> >>>>
> >>>>
> >>>> Rusty, MST, would you be open to a spec and code change that would initialize the
> >>>> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
> >>>> to work around this issue?
> >>>
> >>> I'm confused.  kvmtool is using too much memory, or the guest?  If
> >>> kvmtool, the Device Initialization section above applies to the driver,
> >>> not the device.  If the guest, well, the language says "UP TO N+1".  You
> >>> want a small guest, don't use them all.  Or any...
> >>>
> >>> What am I missing?
> >>
> >> It's in the guest - sorry. I was only trying to say that kvmtool doesn't do anything
> >> odd with regards to initializing virtio-net.
> >>
> >> The thing is that there should be a difference between just allowing a larger number
> >> of queues and actually using them (i.e. enabling them with ethtool). Right now I see
> >> the kernel lose 130MB just by having kvmtool offer 8 queue pairs, without actually
> >> using those queues.
> >>
> >> Yes, we can make it configurable in kvmtool (and I will make it so so the arm folks
> >> could continue working with tiny guests) but does it make sense that you have to do
> >> this configuration in *2* places? First in the hypervisor and then inside the guest?
> >>
> >> Memory usage should ideally depend on whether you are actually using multiple queues,
> >> not on whether you just allow using those queues.
> >>
> >>
> >> Thanks,
> >> Sasha
> > 
> > 8 queues eat up 130MB?  Most of the memory is likely for the buffers?  I
> > think we could easily allocate these lazily as queues are enabled,
> > without protocol changes. It's harder to clean them as there's no way to
> > reset a specific queue, but maybe that' good enough for your purposes?
> > 
> 
> Yup, this is how it looks in the guest right after booting:
> 
> Without virtio-net mq:
> 
> # free
>              total       used       free     shared    buffers     cached
> Mem:        918112     158052     760060          0          0       4308
> -/+ buffers/cache:     153744     764368
> 
> With queue pairs = 8:
> 
> # free
>              total       used       free     shared    buffers     cached
> Mem:        918112     289168     628944          0          0       4244
> -/+ buffers/cache:     284924     633188
> 
> 
> Initializing them only when they're actually needed will do the trick here.

Not initializing, adding the buffers. In the current spec, initialization
is always done before DRIVER_OK.

> We could also expose the facility to delete a single vq, and add a note to
> the spec saying that if the amount of actual vq pairs was reduced below what
> it was before, the now inactive queues become invalid and would need to be
> re-initialized. It's not pretty but it would let both device and driver free
> up those vqs.
> 
> 
> Thanks,
> Sasha

If we want to do this, I would add a command to have
the device complete all RX buffers of a given VQ with len=0.
I wouldn't do this automatically when number of VQs changes since we had
an idea to switch between MQ and SQ mode dynamically,
which needs all buffers to stay in the VQ.


-- 
MST

Re: virtio-net mq vq initialization

[Sasha Levin]

On 04/14/2013 11:53 AM, Michael S. Tsirkin wrote:
>> > 
>> > Initializing them only when they're actually needed will do the trick here.
> Not initializing, adding the buffers. In the current spec, initialization
> is always done before DRIVER_OK.

Yeah, that's better, but we're going to need a spec change either way since even
adding the buffers is specifically stated to happen before DRIVER_OK:

"""
	6. The receive virtqueues should be filled with receive buffers. This is described
in detail below in “Setting Up Receive Buffers”.
"""

Thanks,
Sasha

Re: virtio-net mq vq initialization

[Michael S. Tsirkin]

On Sun, Apr 14, 2013 at 11:59:42AM -0400, Sasha Levin wrote:
> On 04/14/2013 11:53 AM, Michael S. Tsirkin wrote:
> >> > 
> >> > Initializing them only when they're actually needed will do the trick here.
> > Not initializing, adding the buffers. In the current spec, initialization
> > is always done before DRIVER_OK.
> 
> Yeah, that's better, but we're going to need a spec change either way since even
> adding the buffers is specifically stated to happen before DRIVER_OK:
> 
> """
> 	6. The receive virtqueues should be filled with receive buffers. This is described
> in detail below in “Setting Up Receive Buffers”.
> """
> 
> Thanks,
> Sasha

Yes but that's a minor point, specific to virio-net.
The main point was to ensure that VQs should have buffers before they get
any packets.

OTOH VQ init bere DRIVER_OK is a generic virtio thing: DRIVER_OK means
we can start sending interrupts and we expect driver to be ready to get
them.

Let's solve the real problem with buffers, a couple of pages per VQ
shouldn't be an issue.

-- 
MST

Re: virtio-net mq vq initialization

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Sun, Apr 14, 2013 at 11:59:42AM -0400, Sasha Levin wrote:
>> On 04/14/2013 11:53 AM, Michael S. Tsirkin wrote:
>> >> > 
>> >> > Initializing them only when they're actually needed will do the trick here.
>> > Not initializing, adding the buffers. In the current spec, initialization
>> > is always done before DRIVER_OK.
>> 
>> Yeah, that's better, but we're going to need a spec change either way since even
>> adding the buffers is specifically stated to happen before DRIVER_OK:
>> 
>> """
>> 	6. The receive virtqueues should be filled with receive buffers. This is described
>> in detail below in “Setting Up Receive Buffers”.
>> """
>> 
>> Thanks,
>> Sasha
>
> Yes but that's a minor point, specific to virio-net.
> The main point was to ensure that VQs should have buffers before they get
> any packets.
>
> OTOH VQ init bere DRIVER_OK is a generic virtio thing: DRIVER_OK means
> we can start sending interrupts and we expect driver to be ready to get
> them.
>
> Let's solve the real problem with buffers, a couple of pages per VQ
> shouldn't be an issue.

Yeah, let's not populate the extra virtqueues until someone turns
multiqueue on.

I appended to the last paragraph under Setting Up Receive Buffers:

        If VIRTIO_NET_F_MQ is negotiated, each of receiveq0...receiveqN that
        will be used should be populated with receive buffers

Added: 
       before multiqueue is activated (See [sub:Automatic-receive-steering]).

Cheers,
Rusty.

Re: virtio-net mq vq initialization

[Jason Wang]

On 04/14/2013 11:16 PM, Sasha Levin wrote:
> On 04/14/2013 06:01 AM, Michael S. Tsirkin wrote:
>> On Sat, Apr 13, 2013 at 05:23:41PM -0400, Sasha Levin wrote:
>>> On 04/12/2013 07:36 AM, Rusty Russell wrote:
>>>> Sasha Levin <sasha.levin@oracle.com> writes:
>>>>> On 04/11/2013 12:36 PM, Will Deacon wrote:
>>>>>> Hello folks,
>>>>>>
>>>>>> Here's the latest round of ARM fixes and updates for kvmtool. Most of
>>>>>> this is confined to the arm/ subdirectory, with the exception of a fix
>>>>>> to the virtio-mmio vq definitions due to the multi-queue work from
>>>>>> Sasha. I'm not terribly happy about that code though, since it seriously
>>>>>> increases the memory footprint of the guest.
>>>>>>
>>>>>> Without multi-queue, we can boot Debian Wheezy to a prompt in 38MB. With
>>>>>> the new changes, that increases to 170MB! Any chance we can try and tackle
>>>>>> this regression please? I keep getting bitten by the OOM killer :(
>>>>> (cc Rusty, MST)
>>>>>
>>>>> The spec defines the operation of a virtio-net device with regards to multiple
>>>>> queues as follows:
>>>>>
>>>>> """
>>>>> Device Initialization
>>>>>
>>>>> 	1. The initialization routine should identify the receive and transmission
>>>>> virtqueues, up to N+1 of each kind. If VIRTIO_NET_F_MQ feature
>>>>> bit is negotiated, N=max_virtqueue_pairs-1, otherwise identify N=0.
>>>>>
>>>>> 	[...]
>>>>>
>>>>> 	5. Only receiveq0, transmitq0 and controlq are used by default. To use more
>>>>> queues driver must negotiate the VIRTIO_NET_F_MQ feature; initialize
>>>>> up to max_virtqueue_pairs of each of transmit and receive queues; execute_
>>>>> VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command specifying
>>>>> the number of the transmit and receive queues that is going to be
>>>>> used and wait until the device consumes the controlq buffer and acks this
>>>>> command.
>>>>> """
>>>>>
>>>>> And kvmtool follows that to the letter: It will initialize the maximum amount of
>>>>> queues it can support during initialization and will start using them only when
>>>>> the device tells it it should use them.
>>>>>
>>>>> As Will has stated, this causes a memory issue since all the data structures that hold
>>>>> all possible queues get initialized regardless of whether we actually need them or not,
>>>>> which is quite troublesome for systems with small RAM.
>>>>>
>>>>>
>>>>> Rusty, MST, would you be open to a spec and code change that would initialize the
>>>>> RX/TX vqs on demand instead of on device initialization? Or is there an easier way
>>>>> to work around this issue?
>>>> I'm confused.  kvmtool is using too much memory, or the guest?  If
>>>> kvmtool, the Device Initialization section above applies to the driver,
>>>> not the device.  If the guest, well, the language says "UP TO N+1".  You
>>>> want a small guest, don't use them all.  Or any...
>>>>
>>>> What am I missing?
>>> It's in the guest - sorry. I was only trying to say that kvmtool doesn't do anything
>>> odd with regards to initializing virtio-net.
>>>
>>> The thing is that there should be a difference between just allowing a larger number
>>> of queues and actually using them (i.e. enabling them with ethtool). Right now I see
>>> the kernel lose 130MB just by having kvmtool offer 8 queue pairs, without actually
>>> using those queues.
>>>
>>> Yes, we can make it configurable in kvmtool (and I will make it so so the arm folks
>>> could continue working with tiny guests) but does it make sense that you have to do
>>> this configuration in *2* places? First in the hypervisor and then inside the guest?
>>>
>>> Memory usage should ideally depend on whether you are actually using multiple queues,
>>> not on whether you just allow using those queues.
>>>
>>>
>>> Thanks,
>>> Sasha
>> 8 queues eat up 130MB?  Most of the memory is likely for the buffers?  I
>> think we could easily allocate these lazily as queues are enabled,
>> without protocol changes. It's harder to clean them as there's no way to
>> reset a specific queue, but maybe that' good enough for your purposes?
>>
> Yup, this is how it looks in the guest right after booting:
>
> Without virtio-net mq:
>
> # free
>              total       used       free     shared    buffers     cached
> Mem:        918112     158052     760060          0          0       4308
> -/+ buffers/cache:     153744     764368
>
> With queue pairs = 8:
>
> # free
>              total       used       free     shared    buffers     cached
> Mem:        918112     289168     628944          0          0       4244
> -/+ buffers/cache:     284924     633188
>
>
> Initializing them only when they're actually needed will do the trick here.

I don't see so much memory allocation with qemu, the main problem I
guess here is kvmtool does not support mergeable rx buffers ( does it?
). So guest must allocate 64K per packet which would be even worse in
multiqueue case. If mergeable rx buffer was used, the receive buffer
will only occupy about 256 * 4K (1M) which seems pretty acceptable here.
>
> We could also expose the facility to delete a single vq, and add a note to
> the spec saying that if the amount of actual vq pairs was reduced below what
> it was before, the now inactive queues become invalid and would need to be
> re-initialized. It's not pretty but it would let both device and driver free
> up those vqs.
>
>
> Thanks,
> Sasha
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: virtio-net mq vq initialization

[Sasha Levin]

On 04/15/2013 01:58 AM, Jason Wang wrote:
>> Initializing them only when they're actually needed will do the trick here.
> I don't see so much memory allocation with qemu, the main problem I
> guess here is kvmtool does not support mergeable rx buffers ( does it?
> ). So guest must allocate 64K per packet which would be even worse in
> multiqueue case. If mergeable rx buffer was used, the receive buffer
> will only occupy about 256 * 4K (1M) which seems pretty acceptable here.

Yup, kvmtool doesn't do mergable RX at the moment.

I'll add support for that in, thanks for pointing it out!


Thanks,
Sasha