[PATCH 0/2] vhost: add ioctl to query nregions limit and rise default limit

[PATCH 0/2] vhost: add ioctl to query nregions limit and rise default limit

[Igor Mammedov]

Igor Mammedov (1):
  vhost: increase default limit of nregions from 64 to 509

Michael S. Tsirkin (1):
  vhost: add ioctl to query nregions upper limit

 drivers/vhost/vhost.c      |  7 ++++++-
 include/uapi/linux/vhost.h | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Igor Mammedov]

From: "Michael S. Tsirkin" <mst@redhat.com>

Userspace currently simply tries to give vhost as many regions
as it happens to have, but you only have the mem table
when you have initialized a large part of VM, so graceful
failure is very hard to support.

The result is that userspace tends to fail catastrophically.

Instead, add a new ioctl so userspace can find out how much kernel
supports, up front. This returns a positive value that we commit to.

Also, document our contract with legacy userspace: when running on an
old kernel, you get -1 and you can assume at least 64 slots.  Since 0
value's left unused, let's make that mean that the current userspace
behaviour (trial and error) is required, just in case we want it back.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 drivers/vhost/vhost.c      |  7 ++++++-
 include/uapi/linux/vhost.h | 17 ++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index eec2f11..76dc0cf 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -30,7 +30,7 @@
 
 #include "vhost.h"
 
-static ushort max_mem_regions = 64;
+static ushort max_mem_regions = VHOST_MEM_MAX_NREGIONS_DEFAULT;
 module_param(max_mem_regions, ushort, 0444);
 MODULE_PARM_DESC(max_mem_regions,
 	"Maximum number of memory regions in memory map. (default: 64)");
@@ -944,6 +944,11 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 	long r;
 	int i, fd;
 
+	if (ioctl == VHOST_GET_MEM_MAX_NREGIONS) {
+		r = max_mem_regions;
+		goto done;
+	}
+
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
 		r = vhost_dev_set_owner(d);
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index ab373191..2511954 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -80,7 +80,7 @@ struct vhost_memory {
  * Allows subsequent call to VHOST_OWNER_SET to succeed. */
 #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
 
-/* Set up/modify memory layout */
+/* Set up/modify memory layout: see also VHOST_GET_MEM_MAX_NREGIONS below. */
 #define VHOST_SET_MEM_TABLE	_IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
 
 /* Write logging setup. */
@@ -127,6 +127,21 @@ struct vhost_memory {
 /* Set eventfd to signal an error */
 #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
 
+/* Query upper limit on nregions in VHOST_SET_MEM_TABLE arguments.
+ * Returns:
+ *	0 < value <= MAX_INT - gives the upper limit, higher values will fail
+ *	0 - there's no static limit: try and see if it works
+ *	-1 - on failure
+ */
+#define VHOST_GET_MEM_MAX_NREGIONS   _IO(VHOST_VIRTIO, 0x23)
+
+/* Returned by VHOST_GET_MEM_MAX_NREGIONS to mean there's no static limit:
+ * try and it'll work if you are lucky. */
+#define VHOST_MEM_MAX_NREGIONS_NONE 0
+/* We support at least as many nregions in VHOST_SET_MEM_TABLE:
+ * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
+#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
+
 /* VHOST_NET specific defines */
 
 /* Attach virtio net ring to a raw socket, or tap device.
-- 
1.8.3.1

Re: [PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 04:29:22PM +0200, Igor Mammedov wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> 
> Userspace currently simply tries to give vhost as many regions
> as it happens to have, but you only have the mem table
> when you have initialized a large part of VM, so graceful
> failure is very hard to support.
> 
> The result is that userspace tends to fail catastrophically.
> 
> Instead, add a new ioctl so userspace can find out how much kernel
> supports, up front. This returns a positive value that we commit to.
> 
> Also, document our contract with legacy userspace: when running on an
> old kernel, you get -1 and you can assume at least 64 slots.  Since 0
> value's left unused, let's make that mean that the current userspace
> behaviour (trial and error) is required, just in case we want it back.

What's wrong with reading the module parameter value? It's there in
sysfs ...

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  drivers/vhost/vhost.c      |  7 ++++++-
>  include/uapi/linux/vhost.h | 17 ++++++++++++++++-
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index eec2f11..76dc0cf 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -30,7 +30,7 @@
>  
>  #include "vhost.h"
>  
> -static ushort max_mem_regions = 64;
> +static ushort max_mem_regions = VHOST_MEM_MAX_NREGIONS_DEFAULT;
>  module_param(max_mem_regions, ushort, 0444);
>  MODULE_PARM_DESC(max_mem_regions,
>  	"Maximum number of memory regions in memory map. (default: 64)");
> @@ -944,6 +944,11 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  	long r;
>  	int i, fd;
>  
> +	if (ioctl == VHOST_GET_MEM_MAX_NREGIONS) {
> +		r = max_mem_regions;
> +		goto done;
> +	}
> +
>  	/* If you are not the owner, you can become one */
>  	if (ioctl == VHOST_SET_OWNER) {
>  		r = vhost_dev_set_owner(d);
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index ab373191..2511954 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -80,7 +80,7 @@ struct vhost_memory {
>   * Allows subsequent call to VHOST_OWNER_SET to succeed. */
>  #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
>  
> -/* Set up/modify memory layout */
> +/* Set up/modify memory layout: see also VHOST_GET_MEM_MAX_NREGIONS below. */
>  #define VHOST_SET_MEM_TABLE	_IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
>  
>  /* Write logging setup. */
> @@ -127,6 +127,21 @@ struct vhost_memory {
>  /* Set eventfd to signal an error */
>  #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
>  
> +/* Query upper limit on nregions in VHOST_SET_MEM_TABLE arguments.
> + * Returns:
> + *	0 < value <= MAX_INT - gives the upper limit, higher values will fail
> + *	0 - there's no static limit: try and see if it works
> + *	-1 - on failure
> + */
> +#define VHOST_GET_MEM_MAX_NREGIONS   _IO(VHOST_VIRTIO, 0x23)
> +
> +/* Returned by VHOST_GET_MEM_MAX_NREGIONS to mean there's no static limit:
> + * try and it'll work if you are lucky. */
> +#define VHOST_MEM_MAX_NREGIONS_NONE 0
> +/* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> + * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> +
>  /* VHOST_NET specific defines */
>  
>  /* Attach virtio net ring to a raw socket, or tap device.
> -- 
> 1.8.3.1

Re: [PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Igor Mammedov]

On Wed, 29 Jul 2015 17:43:17 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 29, 2015 at 04:29:22PM +0200, Igor Mammedov wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > Userspace currently simply tries to give vhost as many regions
> > as it happens to have, but you only have the mem table
> > when you have initialized a large part of VM, so graceful
> > failure is very hard to support.
> > 
> > The result is that userspace tends to fail catastrophically.
> > 
> > Instead, add a new ioctl so userspace can find out how much kernel
> > supports, up front. This returns a positive value that we commit to.
> > 
> > Also, document our contract with legacy userspace: when running on an
> > old kernel, you get -1 and you can assume at least 64 slots.  Since 0
> > value's left unused, let's make that mean that the current userspace
> > behaviour (trial and error) is required, just in case we want it back.
> 
> What's wrong with reading the module parameter value? It's there in
> sysfs ...
for most cases it would work but distro doesn't have to mount
sysfs under /sys so it adds to app a burden of discovering
where sysfs is mounted and in what module to look for parameter.

So IMHO, sysfs is more human oriented interface,
while ioctl is more stable API for apps.

> 
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  drivers/vhost/vhost.c      |  7 ++++++-
> >  include/uapi/linux/vhost.h | 17 ++++++++++++++++-
> >  2 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index eec2f11..76dc0cf 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -30,7 +30,7 @@
> >  
> >  #include "vhost.h"
> >  
> > -static ushort max_mem_regions = 64;
> > +static ushort max_mem_regions = VHOST_MEM_MAX_NREGIONS_DEFAULT;
> >  module_param(max_mem_regions, ushort, 0444);
> >  MODULE_PARM_DESC(max_mem_regions,
> >  	"Maximum number of memory regions in memory map. (default: 64)");
> > @@ -944,6 +944,11 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> >  	long r;
> >  	int i, fd;
> >  
> > +	if (ioctl == VHOST_GET_MEM_MAX_NREGIONS) {
> > +		r = max_mem_regions;
> > +		goto done;
> > +	}
> > +
> >  	/* If you are not the owner, you can become one */
> >  	if (ioctl == VHOST_SET_OWNER) {
> >  		r = vhost_dev_set_owner(d);
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index ab373191..2511954 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -80,7 +80,7 @@ struct vhost_memory {
> >   * Allows subsequent call to VHOST_OWNER_SET to succeed. */
> >  #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
> >  
> > -/* Set up/modify memory layout */
> > +/* Set up/modify memory layout: see also VHOST_GET_MEM_MAX_NREGIONS below. */
> >  #define VHOST_SET_MEM_TABLE	_IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
> >  
> >  /* Write logging setup. */
> > @@ -127,6 +127,21 @@ struct vhost_memory {
> >  /* Set eventfd to signal an error */
> >  #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
> >  
> > +/* Query upper limit on nregions in VHOST_SET_MEM_TABLE arguments.
> > + * Returns:
> > + *	0 < value <= MAX_INT - gives the upper limit, higher values will fail
> > + *	0 - there's no static limit: try and see if it works
> > + *	-1 - on failure
> > + */
> > +#define VHOST_GET_MEM_MAX_NREGIONS   _IO(VHOST_VIRTIO, 0x23)
> > +
> > +/* Returned by VHOST_GET_MEM_MAX_NREGIONS to mean there's no static limit:
> > + * try and it'll work if you are lucky. */
> > +#define VHOST_MEM_MAX_NREGIONS_NONE 0
> > +/* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> > + * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > +
> >  /* VHOST_NET specific defines */
> >  
> >  /* Attach virtio net ring to a raw socket, or tap device.
> > -- 
> > 1.8.3.1

Re: [PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 04:53:51PM +0200, Igor Mammedov wrote:
> On Wed, 29 Jul 2015 17:43:17 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 29, 2015 at 04:29:22PM +0200, Igor Mammedov wrote:
> > > From: "Michael S. Tsirkin" <mst@redhat.com>
> > > 
> > > Userspace currently simply tries to give vhost as many regions
> > > as it happens to have, but you only have the mem table
> > > when you have initialized a large part of VM, so graceful
> > > failure is very hard to support.
> > > 
> > > The result is that userspace tends to fail catastrophically.
> > > 
> > > Instead, add a new ioctl so userspace can find out how much kernel
> > > supports, up front. This returns a positive value that we commit to.
> > > 
> > > Also, document our contract with legacy userspace: when running on an
> > > old kernel, you get -1 and you can assume at least 64 slots.  Since 0
> > > value's left unused, let's make that mean that the current userspace
> > > behaviour (trial and error) is required, just in case we want it back.
> > 
> > What's wrong with reading the module parameter value? It's there in
> > sysfs ...
> for most cases it would work but distro doesn't have to mount
> sysfs under /sys

If it wants to rewrite all userspace, sure it doesn't.

> so it adds to app a burden of discovering
> where sysfs is mounted and in what module to look for parameter.
> 
> So IMHO, sysfs is more human oriented interface,
> while ioctl is more stable API for apps.

I don't think that's right. ioctls only make sense for per-fd info.

> > 
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  drivers/vhost/vhost.c      |  7 ++++++-
> > >  include/uapi/linux/vhost.h | 17 ++++++++++++++++-
> > >  2 files changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index eec2f11..76dc0cf 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -30,7 +30,7 @@
> > >  
> > >  #include "vhost.h"
> > >  
> > > -static ushort max_mem_regions = 64;
> > > +static ushort max_mem_regions = VHOST_MEM_MAX_NREGIONS_DEFAULT;
> > >  module_param(max_mem_regions, ushort, 0444);
> > >  MODULE_PARM_DESC(max_mem_regions,
> > >  	"Maximum number of memory regions in memory map. (default: 64)");
> > > @@ -944,6 +944,11 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
> > >  	long r;
> > >  	int i, fd;
> > >  
> > > +	if (ioctl == VHOST_GET_MEM_MAX_NREGIONS) {
> > > +		r = max_mem_regions;
> > > +		goto done;
> > > +	}
> > > +
> > >  	/* If you are not the owner, you can become one */
> > >  	if (ioctl == VHOST_SET_OWNER) {
> > >  		r = vhost_dev_set_owner(d);
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index ab373191..2511954 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -80,7 +80,7 @@ struct vhost_memory {
> > >   * Allows subsequent call to VHOST_OWNER_SET to succeed. */
> > >  #define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
> > >  
> > > -/* Set up/modify memory layout */
> > > +/* Set up/modify memory layout: see also VHOST_GET_MEM_MAX_NREGIONS below. */
> > >  #define VHOST_SET_MEM_TABLE	_IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
> > >  
> > >  /* Write logging setup. */
> > > @@ -127,6 +127,21 @@ struct vhost_memory {
> > >  /* Set eventfd to signal an error */
> > >  #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
> > >  
> > > +/* Query upper limit on nregions in VHOST_SET_MEM_TABLE arguments.
> > > + * Returns:
> > > + *	0 < value <= MAX_INT - gives the upper limit, higher values will fail
> > > + *	0 - there's no static limit: try and see if it works
> > > + *	-1 - on failure
> > > + */
> > > +#define VHOST_GET_MEM_MAX_NREGIONS   _IO(VHOST_VIRTIO, 0x23)
> > > +
> > > +/* Returned by VHOST_GET_MEM_MAX_NREGIONS to mean there's no static limit:
> > > + * try and it'll work if you are lucky. */
> > > +#define VHOST_MEM_MAX_NREGIONS_NONE 0
> > > +/* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> > > + * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > > +
> > >  /* VHOST_NET specific defines */
> > >  
> > >  /* Attach virtio net ring to a raw socket, or tap device.
> > > -- 
> > > 1.8.3.1

Re: [PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Paolo Bonzini]

On 29/07/2015 16:56, Michael S. Tsirkin wrote:
>>>> > > > Also, document our contract with legacy userspace: when running on an
>>>> > > > old kernel, you get -1 and you can assume at least 64 slots.  Since 0
>>>> > > > value's left unused, let's make that mean that the current userspace
>>>> > > > behaviour (trial and error) is required, just in case we want it back.
>>> > > 
>>> > > What's wrong with reading the module parameter value? It's there in
>>> > > sysfs ...
>> > for most cases it would work but distro doesn't have to mount
>> > sysfs under /sys
> If it wants to rewrite all userspace, sure it doesn't.

I agree, on the other hand it doesn't seem far fetched to have a per-fd
maximum in the future.  So I think this patch is more future-proof.

Paolo

Re: [PATCH 1/2] vhost: add ioctl to query nregions upper limit

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 05:01:43PM +0200, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 16:56, Michael S. Tsirkin wrote:
> >>>> > > > Also, document our contract with legacy userspace: when running on an
> >>>> > > > old kernel, you get -1 and you can assume at least 64 slots.  Since 0
> >>>> > > > value's left unused, let's make that mean that the current userspace
> >>>> > > > behaviour (trial and error) is required, just in case we want it back.
> >>> > > 
> >>> > > What's wrong with reading the module parameter value? It's there in
> >>> > > sysfs ...
> >> > for most cases it would work but distro doesn't have to mount
> >> > sysfs under /sys
> > If it wants to rewrite all userspace, sure it doesn't.
> 
> I agree, on the other hand it doesn't seem far fetched to have a per-fd
> maximum in the future.  So I think this patch is more future-proof.
> 
> Paolo

Possibly, but this calls for some kind of priveledge separation model,
where a priveledged app can set the per-fd limit while regular ones
can only read it. Sounds very complex. Let's see some of this code
first.

And I really think there are better ways to future proof it, e.g. teach
userspace to do error handling, revert adding a slot if one of the
components can't support it.

-- 
MST

[PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Igor Mammedov]

although now there is vhost module max_mem_regions option
to set custom limit it doesn't help for default setups,
since it requires administrator manually set a higher
limit on each host. Which complicates servers deployments
and management.
Rise limit to the same value as KVM has (509 slots max),
so that default deployments would work out of box.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
Users that would want to lock down vhost could still
use max_mem_regions option to set lower limit, but
I expect it would be minority.
---
 include/uapi/linux/vhost.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index 2511954..92657bf 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -140,7 +140,7 @@ struct vhost_memory {
 #define VHOST_MEM_MAX_NREGIONS_NONE 0
 /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
  * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
-#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
+#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
 
 /* VHOST_NET specific defines */
 
-- 
1.8.3.1

Re: [PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Michael S. Tsirkin]

On Wed, Jul 29, 2015 at 04:29:23PM +0200, Igor Mammedov wrote:
> although now there is vhost module max_mem_regions option
> to set custom limit it doesn't help for default setups,
> since it requires administrator manually set a higher
> limit on each host. Which complicates servers deployments
> and management.
> Rise limit to the same value as KVM has (509 slots max),
> so that default deployments would work out of box.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
> Users that would want to lock down vhost could still
> use max_mem_regions option to set lower limit, but
> I expect it would be minority.

I'm not inclined to merge this.

Once we change this we can't take it back. It's not a decision
to be taken lightly.

And memory hotplug users are a minority.  Out of these, users with a
heavily fragmented PA space due to hotplug abuse are an even smaller
minority.

> ---
>  include/uapi/linux/vhost.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index 2511954..92657bf 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -140,7 +140,7 @@ struct vhost_memory {
>  #define VHOST_MEM_MAX_NREGIONS_NONE 0
>  /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
>   * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> -#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
>  
>  /* VHOST_NET specific defines */
>  
> -- 
> 1.8.3.1

Re: [PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Igor Mammedov]

On Wed, 29 Jul 2015 18:28:26 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jul 29, 2015 at 04:29:23PM +0200, Igor Mammedov wrote:
> > although now there is vhost module max_mem_regions option
> > to set custom limit it doesn't help for default setups,
> > since it requires administrator manually set a higher
> > limit on each host. Which complicates servers deployments
> > and management.
> > Rise limit to the same value as KVM has (509 slots max),
> > so that default deployments would work out of box.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > PS:
> > Users that would want to lock down vhost could still
> > use max_mem_regions option to set lower limit, but
> > I expect it would be minority.
> 
> I'm not inclined to merge this.
> 
> Once we change this we can't take it back. It's not a decision
> to be taken lightly.
considering that continuous HVA idea has failed, why would you
want to take limit back in the future if we rise it now?

> 
> And memory hotplug users are a minority.  Out of these, users with a
> heavily fragmented PA space due to hotplug abuse are an even smaller
> minority.
> 
> > ---
> >  include/uapi/linux/vhost.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index 2511954..92657bf 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -140,7 +140,7 @@ struct vhost_memory {
> >  #define VHOST_MEM_MAX_NREGIONS_NONE 0
> >  /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> >   * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > -#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
> >  
> >  /* VHOST_NET specific defines */
> >  
> > -- 
> > 1.8.3.1

Re: [PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Michael S. Tsirkin]

On Thu, Jul 30, 2015 at 08:26:03AM +0200, Igor Mammedov wrote:
> On Wed, 29 Jul 2015 18:28:26 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jul 29, 2015 at 04:29:23PM +0200, Igor Mammedov wrote:
> > > although now there is vhost module max_mem_regions option
> > > to set custom limit it doesn't help for default setups,
> > > since it requires administrator manually set a higher
> > > limit on each host. Which complicates servers deployments
> > > and management.
> > > Rise limit to the same value as KVM has (509 slots max),
> > > so that default deployments would work out of box.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > PS:
> > > Users that would want to lock down vhost could still
> > > use max_mem_regions option to set lower limit, but
> > > I expect it would be minority.
> > 
> > I'm not inclined to merge this.
> > 
> > Once we change this we can't take it back. It's not a decision
> > to be taken lightly.
> considering that continuous HVA idea has failed, why would you
> want to take limit back in the future if we rise it now?

I'm not sure.

I think you merely demonstrated it's a big change for userspace -
not that it's unfeasible.

Alternatively, if we want an unlimited size table, we should keep it
in userspace memory.

> > 
> > And memory hotplug users are a minority.  Out of these, users with a
> > heavily fragmented PA space due to hotplug abuse are an even smaller
> > minority.
> > 
> > > ---
> > >  include/uapi/linux/vhost.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index 2511954..92657bf 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -140,7 +140,7 @@ struct vhost_memory {
> > >  #define VHOST_MEM_MAX_NREGIONS_NONE 0
> > >  /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> > >   * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > > -#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
> > >  
> > >  /* VHOST_NET specific defines */
> > >  
> > > -- 
> > > 1.8.3.1

Re: [PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Igor Mammedov]

On Thu, 30 Jul 2015 09:33:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 08:26:03AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Jul 2015 18:28:26 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 29, 2015 at 04:29:23PM +0200, Igor Mammedov wrote:
> > > > although now there is vhost module max_mem_regions option
> > > > to set custom limit it doesn't help for default setups,
> > > > since it requires administrator manually set a higher
> > > > limit on each host. Which complicates servers deployments
> > > > and management.
> > > > Rise limit to the same value as KVM has (509 slots max),
> > > > so that default deployments would work out of box.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > PS:
> > > > Users that would want to lock down vhost could still
> > > > use max_mem_regions option to set lower limit, but
> > > > I expect it would be minority.
> > > 
> > > I'm not inclined to merge this.
> > > 
> > > Once we change this we can't take it back. It's not a decision
> > > to be taken lightly.
in addition, you already gave out control on limit allowing
to rise it via module parameter. Rising default is just a way
to reduce pain for users that would try to use more than 64
slots.

> > considering that continuous HVA idea has failed, why would you
> > want to take limit back in the future if we rise it now?
> 
> I'm not sure.
> 
> I think you merely demonstrated it's a big change for userspace -
> not that it's unfeasible.
It's not a big change but rather ugly one, being unportable,
enforcing unnecessary (and to really reasonable) restrictions
on memory backends and changing memory unplug mgmt workflow
depending on if HVA used or not.

> Alternatively, if we want an unlimited size table, we should keep it
> in userspace memory.
this patch doesn't propose unlimited table size.
with proposed limit we are talking about max order-4 allocations
that can fallback to vmalloc if needed. And it makes vhost consistent
with KVM's limit, which has similar table.
Proposed limit also neatly works around corner cases of
existing old userspace that can't deal with it when it hits limit.


> 
> > > 
> > > And memory hotplug users are a minority.  Out of these, users with a
> > > heavily fragmented PA space due to hotplug abuse are an even smaller
> > > minority.
> > > 
> > > > ---
> > > >  include/uapi/linux/vhost.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > index 2511954..92657bf 100644
> > > > --- a/include/uapi/linux/vhost.h
> > > > +++ b/include/uapi/linux/vhost.h
> > > > @@ -140,7 +140,7 @@ struct vhost_memory {
> > > >  #define VHOST_MEM_MAX_NREGIONS_NONE 0
> > > >  /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> > > >   * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > > > -#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > > > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
> > > >  
> > > >  /* VHOST_NET specific defines */
> > > >  
> > > > -- 
> > > > 1.8.3.1

Re: [PATCH 2/2] vhost: increase default limit of nregions from 64 to 509

[Igor Mammedov]

On Thu, 30 Jul 2015 09:33:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 30, 2015 at 08:26:03AM +0200, Igor Mammedov wrote:
> > On Wed, 29 Jul 2015 18:28:26 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Wed, Jul 29, 2015 at 04:29:23PM +0200, Igor Mammedov wrote:
> > > > although now there is vhost module max_mem_regions option
> > > > to set custom limit it doesn't help for default setups,
> > > > since it requires administrator manually set a higher
> > > > limit on each host. Which complicates servers deployments
> > > > and management.
> > > > Rise limit to the same value as KVM has (509 slots max),
> > > > so that default deployments would work out of box.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > PS:
> > > > Users that would want to lock down vhost could still
> > > > use max_mem_regions option to set lower limit, but
> > > > I expect it would be minority.
> > > 
> > > I'm not inclined to merge this.
> > > 
> > > Once we change this we can't take it back. It's not a decision
> > > to be taken lightly.
> > considering that continuous HVA idea has failed, why would you
> > want to take limit back in the future if we rise it now?
> 
> I'm not sure.
> 
> I think you merely demonstrated it's a big change for userspace -
> not that it's unfeasible.
> 
> Alternatively, if we want an unlimited size table, we should keep it
> in userspace memory.
btw:
if table were a simple array and kernel will do inefficient linear scan
to do translation then I guess we could use userspace memory.

But I'm afraid we can't trust userspace in case of more elaborate
structure. Even if it's just binary search over sorted array,
it would be possible for userspace to hung kernel thread in
translate_desc() by providing corrupted or wrongly sorted table.
And we can't afford table validation on hot path.


> 
> > > 
> > > And memory hotplug users are a minority.  Out of these, users with a
> > > heavily fragmented PA space due to hotplug abuse are an even smaller
> > > minority.
> > > 
> > > > ---
> > > >  include/uapi/linux/vhost.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > index 2511954..92657bf 100644
> > > > --- a/include/uapi/linux/vhost.h
> > > > +++ b/include/uapi/linux/vhost.h
> > > > @@ -140,7 +140,7 @@ struct vhost_memory {
> > > >  #define VHOST_MEM_MAX_NREGIONS_NONE 0
> > > >  /* We support at least as many nregions in VHOST_SET_MEM_TABLE:
> > > >   * for use on legacy kernels without VHOST_GET_MEM_MAX_NREGIONS support. */
> > > > -#define VHOST_MEM_MAX_NREGIONS_DEFAULT 64
> > > > +#define VHOST_MEM_MAX_NREGIONS_DEFAULT 509
> > > >  
> > > >  /* VHOST_NET specific defines */
> > > >  
> > > > -- 
> > > > 1.8.3.1
> --
> 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