1. Patchew
  2. Libvirt
  3. View series

[PATCH] conf: check vhost-user queues with vcpus

Jiang Jiacheng posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20221208125219.2940088-1-jiangjiacheng@huawei.com
src/conf/domain_validate.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[PATCH] conf: check vhost-user queues with vcpus
Posted by Jiang Jiacheng 1 year, 4 months ago 
With kernel without the ref patch, if queues > vcpus, interrupts
will be centralized on one vcpu affecting guest performance. After
the ref patch merged, the queues whose number is greater than the
number of vcpus will not be used.

Considering the above, it's better to check the counts of vhost-user
queues and vcpus.

ref:
https://patchwork.kernel.org/project/linux-scsi/cover/1553682995-5682-1-git-send-email-dongli.zhang@oracle.com/

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 src/conf/domain_validate.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 95b8d9b419..6106e79999 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -308,7 +308,7 @@ virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDef **seclabels,
 
 
 static int
-virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
+virDomainDiskVhostUserValidate(const virDomainDef *def, const virDomainDiskDef *disk)
 {
     if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -465,6 +465,12 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
         return -1;
     }
 
+    if (disk->queues > virDomainDefGetVcpus(def)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("vhost-user disk queues must <= vcpus"));
+        return -1;
+    }
+
     return 0;
 }
 
@@ -807,7 +813,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
     }
 
     if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER &&
-        virDomainDiskVhostUserValidate(disk) < 0) {
+        virDomainDiskVhostUserValidate(def, disk) < 0) {
         return -1;
     }
 
-- 
2.33.0
Re: [PATCH] conf: check vhost-user queues with vcpus
Posted by Martin Kletzander 1 year, 3 months ago 
The ping for this patch got threaded weirdly, so I just stumbled upon
this now.

The subject could use s/with/against/ or could be something like "Limit
the number of vhost-user queues to the number of vcpus", but a question
below.

On Thu, Dec 08, 2022 at 08:52:19PM +0800, Jiang Jiacheng wrote:
>With kernel without the ref patch, if queues > vcpus, interrupts
>will be centralized on one vcpu affecting guest performance. After
>the ref patch merged, the queues whose number is greater than the
>number of vcpus will not be used.
>
>Considering the above, it's better to check the counts of vhost-user
>queues and vcpus.
>
>ref:
>https://patchwork.kernel.org/project/linux-scsi/cover/1553682995-5682-1-git-send-email-dongli.zhang@oracle.com/
>

It makes sense, although even from the kernel patch I am not sure
whether this should be checked against maxvcpus (i.e. using
virDomainDefGetVcpusMax()) or whether this is more related to the number
of physical CPUs on the host.  Definitely not the number of online guest
vcpus.

>Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>---
> src/conf/domain_validate.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>index 95b8d9b419..6106e79999 100644
>--- a/src/conf/domain_validate.c
>+++ b/src/conf/domain_validate.c
>@@ -308,7 +308,7 @@ virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDef **seclabels,
>
>
> static int
>-virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
>+virDomainDiskVhostUserValidate(const virDomainDef *def, const virDomainDiskDef *disk)
> {
>     if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
>         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>@@ -465,6 +465,12 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
>         return -1;
>     }
>
>+    if (disk->queues > virDomainDefGetVcpus(def)) {
>+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                       _("vhost-user disk queues must <= vcpus"));
>+        return -1;
>+    }
>+
>     return 0;
> }
>
>@@ -807,7 +813,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
>     }
>
>     if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER &&
>-        virDomainDiskVhostUserValidate(disk) < 0) {
>+        virDomainDiskVhostUserValidate(def, disk) < 0) {
>         return -1;
>     }
>
>-- 
>2.33.0
>
Re: [PATCH] conf: check vhost-user queues with vcpus
Posted by Jiang Jiacheng 1 year, 3 months ago 

On 2023/1/6 17:42, Martin Kletzander wrote:
> The ping for this patch got threaded weirdly, so I just stumbled upon
> this now.
> 
> The subject could use s/with/against/ or could be something like "Limit
> the number of vhost-user queues to the number of vcpus", but a question
> below.
> 
> On Thu, Dec 08, 2022 at 08:52:19PM +0800, Jiang Jiacheng wrote:
>> With kernel without the ref patch, if queues > vcpus, interrupts
>> will be centralized on one vcpu affecting guest performance. After
>> the ref patch merged, the queues whose number is greater than the
>> number of vcpus will not be used.
>>
>> Considering the above, it's better to check the counts of vhost-user
>> queues and vcpus.
>>
>> ref:
>> https://patchwork.kernel.org/project/linux-scsi/cover/1553682995-5682-1-git-send-email-dongli.zhang@oracle.com/
>>
> 
> It makes sense, although even from the kernel patch I am not sure
> whether this should be checked against maxvcpus (i.e. using
> virDomainDefGetVcpusMax()) or whether this is more related to the number
> of physical CPUs on the host.  Definitely not the number of online guest
> vcpus.
> 

Thanks for your reply！

I see, check against online guest vcpus is inappropriate anyway. We have
to take maxvcpus and vcpu hotplug into consideration.
More tests is needed before next version, the max number of queues
should depend on physical CPUs or vcpus, and the interaction between the
queues and vcpu hotplug.
I will try to look for a more appropriate way and attach my test to next
version.

Thank
Jiang Jiacheng
>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>> ---
>> src/conf/domain_validate.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
>> index 95b8d9b419..6106e79999 100644
>> --- a/src/conf/domain_validate.c
>> +++ b/src/conf/domain_validate.c
>> @@ -308,7 +308,7 @@
>> virSecurityDeviceLabelDefValidate(virSecurityDeviceLabelDef **seclabels,
>>
>>
>> static int
>> -virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
>> +virDomainDiskVhostUserValidate(const virDomainDef *def, const
>> virDomainDiskDef *disk)
>> {
>>     if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
>>         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> @@ -465,6 +465,12 @@ virDomainDiskVhostUserValidate(const
>> virDomainDiskDef *disk)
>>         return -1;
>>     }
>>
>> +    if (disk->queues > virDomainDefGetVcpus(def)) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("vhost-user disk queues must <= vcpus"));
>> +        return -1;
>> +    }
>> +
>>     return 0;
>> }
>>
>> @@ -807,7 +813,7 @@ virDomainDiskDefValidate(const virDomainDef *def,
>>     }
>>
>>     if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER &&
>> -        virDomainDiskVhostUserValidate(disk) < 0) {
>> +        virDomainDiskVhostUserValidate(def, disk) < 0) {
>>         return -1;
>>     }
>>
>> -- 
>> 2.33.0
>>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.