[libvirt] [PATCH] rng: tighten up domain <controller> schema
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) and <master> subelement to be included for any controller type. In reality, half of the models are allowed only for type='scsi', and the other half only for type='usb', num_queues is allowed only for type='scsi', and <master> only for type='usb'. This patch makes a separate <group> for type='scsi' and type='usb', with each group allowing only the appropriate model values, and allowing num_queue and <master> only when appropriate. <interleave> also hadn't been specified, forcing a specific order of subelements, which should never be done. (Note that the <interleave> had to surround the main element attributes that are in the <group> subelements, due to one of the <group>s containing a subelement). --- docs/schemas/domaincommon.rng | 145 +++++++++++++++++++++++------------------- 1 file changed, 81 insertions(+), 64 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c96a247..26523a7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1402,80 +1402,97 @@ </define> <define name="controller"> <element name="controller"> - <choice> - <group> - <optional> + <attribute name="index"> + <ref name="unsignedInt"/> + </attribute> + <interleave> + <optional> + <ref name="alias"/> + </optional> + <optional> + <ref name="address"/> + </optional> + <choice> + <!-- fdc/ide/sata/ccid have only the common attributes --> + <group> <attribute name="type"> <choice> <value>fdc</value> <value>ide</value> - <value>scsi</value> <value>sata</value> <value>ccid</value> - <value>usb</value> </choice> </attribute> - </optional> - </group> - <!-- virtio-serial can have 2 additional attributes --> - <group> - <attribute name="type"> - <value>virtio-serial</value> - </attribute> - <optional> - <attribute name="ports"> - <ref name="unsignedInt"/> + </group> + <!-- scsi has an optional attribute "model" --> + <group> + <attribute name="type"> + <value>scsi</value> </attribute> - </optional> - <optional> - <attribute name="vectors"> - <ref name="unsignedInt"/> + <optional> + <attribute name="model"> + <choice> + <value>auto</value> + <value>buslogic</value> + <value>lsilogic</value> + <value>lsisas1068</value> + <value>vmpvscsi</value> + <value>ibmvscsi</value> + <value>virtio-scsi</value> + <value>lsisas1078</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="num_queues"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </group> + <!-- usb has an optional attribute "model", and optional subelement "master" --> + <group> + <attribute name="type"> + <value>usb</value> </attribute> - </optional> - </group> - </choice> - <attribute name="index"> - <ref name="unsignedInt"/> - </attribute> - <optional> - <attribute name="model"> - <choice> - <value>auto</value> - <value>buslogic</value> - <value>lsilogic</value> - <value>lsisas1068</value> - <value>vmpvscsi</value> - <value>ibmvscsi</value> - <value>virtio-scsi</value> - <value>lsisas1078</value> - <value>piix3-uhci</value> - <value>piix4-uhci</value> - <value>ehci</value> - <value>ich9-ehci1</value> - <value>ich9-uhci1</value> - <value>ich9-uhci2</value> - <value>ich9-uhci3</value> - <value>vt82c686b-uhci</value> - <value>pci-ohci</value> - <value>nec-xhci</value> - <value>none</value> - </choice> - </attribute> - </optional> - <optional> - <attribute name="num_queues"> - <ref name="unsignedInt"/> - </attribute> - </optional> - <optional> - <ref name="usbmaster"/> - </optional> - <optional> - <ref name="alias"/> - </optional> - <optional> - <ref name="address"/> - </optional> + <optional> + <attribute name="model"> + <choice> + <value>piix3-uhci</value> + <value>piix4-uhci</value> + <value>ehci</value> + <value>ich9-ehci1</value> + <value>ich9-uhci1</value> + <value>ich9-uhci2</value> + <value>ich9-uhci3</value> + <value>vt82c686b-uhci</value> + <value>pci-ohci</value> + <value>nec-xhci</value> + <value>none</value> + </choice> + </attribute> + </optional> + <optional> + <ref name="usbmaster"/> + </optional> + </group> + <!-- virtio-serial has optional "ports" and "vectors" --> + <group> + <attribute name="type"> + <value>virtio-serial</value> + </attribute> + <optional> + <attribute name="ports"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="vectors"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </group> + </choice> + </interleave> </element> </define> <define name="filesystem"> -- 1.7.11.7
On 04/18/2013 06:36 AM, Laine Stump wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?)
Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I think we should rename it ASAP since we are using camelCase for all the attribute names. Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. Martin
On 18/04/13 16:42, Martin Kletzander wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I
On 04/18/2013 06:36 AM, Laine Stump wrote: think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Osier
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I
On 04/18/2013 06:36 AM, Laine Stump wrote: think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime. Thanks, Martin
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I
On 04/18/2013 06:36 AM, Laine Stump wrote: think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet. It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat. Osier
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I
On 04/18/2013 06:36 AM, Laine Stump wrote: think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. Underscore is not such a deal breaker, yes. Let's see what will be the opinion on the patch I've sent [3]. Martin [1] I've found out only these: - logical_block_size - physical_block_size - error_policy - rerror_policy - event_idx - copy_on_read vendor_id [2] http://www.redhat.com/archives/libvir-list/2012-September/msg01411.html [3] http://www.redhat.com/archives/libvir-list/2013-April/msg01340.html
On 04/18/2013 05:41 AM, Martin Kletzander wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
The rng schema for <controller> had been non-specific about which types of controllers allowed which models, and also allowed the num_queues attribute (since that hasn't been released yet, should we rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I
On 04/18/2013 06:36 AM, Laine Stump wrote: think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2].
I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or the other, but if we really are trying to avoid them, now is our chance). In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? (sorry for not bringing all this up when the patches were originally posted - it's just become impossible for me to even open all the messages on the list, much less read and understand them :-/)
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote:
On 04/18/2013 06:36 AM, Laine Stump wrote: > The rng schema for <controller> had been non-specific about which > types of controllers allowed which models, and also allowed the > num_queues attribute (since that hasn't been released yet, should we > rename it to "numQueues"?) Since there's still time (the commit with that is v1.0.4-65-gd4bf0a9), I think we should rename it ASAP since we are using camelCase for all the attribute names.
Apart from that, the RNG with this patch is precise according to the documentation, so ACK. I'll try to send the numQueues patch to see what others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance).
I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God?
Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
(sorry for not bringing all this up when the patches were originally posted - it's just become impossible for me to even open all the messages on the list, much less read and understand them :-/)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote:
On 18/04/13 16:42, Martin Kletzander wrote: > On 04/18/2013 06:36 AM, Laine Stump wrote: >> The rng schema for <controller> had been non-specific about which >> types of controllers allowed which models, and also allowed the >> num_queues attribute (since that hasn't been released yet, >> should we >> rename it to "numQueues"?) > Since there's still time (the commit with that is > v1.0.4-65-gd4bf0a9), I > think we should rename it ASAP since we are using camelCase for > all the > attribute names. > > Apart from that, the RNG with this patch is precise according to > the > documentation, so ACK. I'll try to send the numQueues patch to see > what > others think. I guess you mean multiple queues support for virtio network? Regardless of which style we will use finally, FYI, "num_queues" is used for disk.. Personally I'm fine with either, because we already use both across.
Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance).
I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God?
Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement. (Just to confuse things a bit, it's actually the <driver> subelement where most of the underscored names live, and they were probably named with underscores for exactly the same reason you named num_queues - because that's the name used in qemu).
On Thu, Apr 18, 2013 at 07:59:45AM -0400, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote: > On 18/04/13 16:42, Martin Kletzander wrote: >> On 04/18/2013 06:36 AM, Laine Stump wrote: >>> The rng schema for <controller> had been non-specific about which >>> types of controllers allowed which models, and also allowed the >>> num_queues attribute (since that hasn't been released yet, >>> should we >>> rename it to "numQueues"?) >> Since there's still time (the commit with that is >> v1.0.4-65-gd4bf0a9), I >> think we should rename it ASAP since we are using camelCase for >> all the >> attribute names. >> >> Apart from that, the RNG with this patch is precise according to >> the >> documentation, so ACK. I'll try to send the numQueues patch to see >> what >> others think. > I guess you mean multiple queues support for virtio network? > Regardless of which style we will use finally, FYI, "num_queues" is > used for disk.. Personally I'm fine with either, because we already > use both across. > Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance).
Yes, we should avoid underscores in all places.
I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God?
Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
(Just to confuse things a bit, it's actually the <driver> subelement where most of the underscored names live, and they were probably named with underscores for exactly the same reason you named num_queues - because that's the name used in qemu).
To be clear, QEMU's choice of underscores for naming its properties has absolutely *ZERO* influence on libvirt's choice of naming. It is a non-Goal to match QEMU's naming. So QEMU's use of underscores, does not imply that libvirt should do the same. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote: > On 18/04/13 16:42, Martin Kletzander wrote: >> On 04/18/2013 06:36 AM, Laine Stump wrote: >>> The rng schema for <controller> had been non-specific about which >>> types of controllers allowed which models, and also allowed the >>> num_queues attribute (since that hasn't been released yet, >>> should we >>> rename it to "numQueues"?) >> Since there's still time (the commit with that is >> v1.0.4-65-gd4bf0a9), I >> think we should rename it ASAP since we are using camelCase for >> all the >> attribute names. >> >> Apart from that, the RNG with this patch is precise according to >> the >> documentation, so ACK. I'll try to send the numQueues patch to see >> what >> others think. > I guess you mean multiple queues support for virtio network? > Regardless of which style we will use finally, FYI, "num_queues" is > used for disk.. Personally I'm fine with either, because we already > use both across. > Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...> And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...> This is the properties the QEMU device "scsi-disk" supports: % ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32 And the properties "virtio-blk-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off And the properties "virtio-scsi-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller. Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go... Osier
Ignore the other 2 copies, they are same. I was blocked by the mail system... On 19/04/13 16:32, Osier Yang wrote:
On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote: > On 04/18/2013 10:54 AM, Osier Yang wrote: >> On 18/04/13 16:42, Martin Kletzander wrote: >>> On 04/18/2013 06:36 AM, Laine Stump wrote: >>>> The rng schema for <controller> had been non-specific about >>>> which >>>> types of controllers allowed which models, and also allowed the >>>> num_queues attribute (since that hasn't been released yet, >>>> should we >>>> rename it to "numQueues"?) >>> Since there's still time (the commit with that is >>> v1.0.4-65-gd4bf0a9), I >>> think we should rename it ASAP since we are using camelCase for >>> all the >>> attribute names. >>> >>> Apart from that, the RNG with this patch is precise according to >>> the >>> documentation, so ACK. I'll try to send the numQueues patch >>> to see >>> what >>> others think. >> I guess you mean multiple queues support for virtio network? >> Regardless of which style we will use finally, FYI, >> "num_queues" is >> used for disk.. Personally I'm fine with either, because we >> already >> use both across. >> > Yes, I meant the virtio-scsi num_queues. As we're trying not to > use > underscores in XML, I hope we can still switch it. I haven't > found any > other num_queues anywhere in the code. Could you point me to the > commit > that uses that? I'm sending the previously discussed patch in the > meantime. > Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...>
And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...>
This is the properties the QEMU device "scsi-disk" supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32
And the properties "virtio-blk-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off
And the properties "virtio-scsi-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off
We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller.
Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go...
Osier
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 04/19/2013 04:32 AM, Osier Yang wrote:
On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote: > On 04/18/2013 10:54 AM, Osier Yang wrote: >> On 18/04/13 16:42, Martin Kletzander wrote: >>> On 04/18/2013 06:36 AM, Laine Stump wrote: >>>> The rng schema for <controller> had been non-specific about >>>> which >>>> types of controllers allowed which models, and also allowed the >>>> num_queues attribute (since that hasn't been released yet, >>>> should we >>>> rename it to "numQueues"?) >>> Since there's still time (the commit with that is >>> v1.0.4-65-gd4bf0a9), I >>> think we should rename it ASAP since we are using camelCase for >>> all the >>> attribute names. >>> >>> Apart from that, the RNG with this patch is precise according to >>> the >>> documentation, so ACK. I'll try to send the numQueues patch >>> to see >>> what >>> others think. >> I guess you mean multiple queues support for virtio network? >> Regardless of which style we will use finally, FYI, >> "num_queues" is >> used for disk.. Personally I'm fine with either, because we >> already >> use both across. >> > Yes, I meant the virtio-scsi num_queues. As we're trying not to > use > underscores in XML, I hope we can still switch it. I haven't > found any > other num_queues anywhere in the code. Could you point me to the > commit > that uses that? I'm sending the previously discussed patch in the > meantime. > Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...>
And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...>
This is the properties the QEMU device "scsi-disk" supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32
And the properties "virtio-blk-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off
And the properties "virtio-scsi-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off
We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller.
Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go...
Wait. So you're saying that num_queues is a property of the *controller* and not of the individual disk, but you've put the config option in the <disk> rather than the <controller>? Why would you do that? If it's a property of the controller, put the tuning parameter in <controller>. Otherwise, what do you do when one <disk> is configured for num_queues=10 and another disk on the same controller is configured for num_queues=2? (And even if you didn't move the config to <controller> (where it seems to me it belongs), moving to the <driver> subelement would still be appropriate - it's still a "backend-specific tuning parameter").
On 04/19/2013 11:25 AM, Laine Stump wrote:
On 18/04/13 19:59, Laine Stump wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote: > On 18/04/13 17:00, Martin Kletzander wrote: >> On 04/18/2013 10:54 AM, Osier Yang wrote: >>> On 18/04/13 16:42, Martin Kletzander wrote: >>>> On 04/18/2013 06:36 AM, Laine Stump wrote: >>>>> The rng schema for <controller> had been non-specific about >>>>> which >>>>> types of controllers allowed which models, and also allowed the >>>>> num_queues attribute (since that hasn't been released yet, >>>>> should we >>>>> rename it to "numQueues"?) >>>> Since there's still time (the commit with that is >>>> v1.0.4-65-gd4bf0a9), I >>>> think we should rename it ASAP since we are using camelCase for >>>> all the >>>> attribute names. >>>> >>>> Apart from that, the RNG with this patch is precise according to >>>> the >>>> documentation, so ACK. I'll try to send the numQueues patch >>>> to see >>>> what >>>> others think. >>> I guess you mean multiple queues support for virtio network? >>> Regardless of which style we will use finally, FYI, >>> "num_queues" is >>> used for disk.. Personally I'm fine with either, because we >>> already >>> use both across. >>> >> Yes, I meant the virtio-scsi num_queues. As we're trying not to >> use >> underscores in XML, I hope we can still switch it. I haven't >> found any >> other num_queues anywhere in the code. Could you point me to the >> commit >> that uses that? I'm sending the previously discussed patch in the >> meantime. >> > Except the virtio-scsi num_queues, there is no other tag for > multiple > queue yet, we will need a patch to support multiple queue for the > network, > but it's not committed yet. > > It's fine to convert it now, 1.0.5 is not released yet. But is it > deserved to > do, we already have many tags with underscore, which can't be > changed > for back-compat. > I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose. But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a
On 04/18/2013 07:27 AM, Osier Yang wrote: particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement. This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...>
And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...>
This is the properties the QEMU device "scsi-disk" supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32
And the properties "virtio-blk-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off
And the properties "virtio-scsi-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off
We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller.
Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go... Wait. So you're saying that num_queues is a property of the *controller* and not of the individual disk, but you've put the config option in the <disk> rather than the <controller>? Why would you do that? If it's a
On 04/19/2013 04:32 AM, Osier Yang wrote: property of the controller, put the tuning parameter in <controller>. Otherwise, what do you do when one <disk> is configured for num_queues=10 and another disk on the same controller is configured for num_queues=2?
Okay, I misunderstood what you said - you weren't saying that you had put num_queues in the <disk> element (obviously - if I was able to retain enough context in my brain to remember the beginning of the thread, I would have known that :-P), but were instead suggesting that I had meant the num_queues should go in the <driver> subelement of <disk>. You misunderstood me (so we're even :-). What I was saying was that it should go in the <driver> subelement of <controller>. I still stand by that opinion.
On 22/04/13 22:00, Laine Stump wrote:
On 04/19/2013 11:25 AM, Laine Stump wrote:
On 18/04/13 19:59, Laine Stump wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 05:41 AM, Martin Kletzander wrote: > On 04/18/2013 11:05 AM, Osier Yang wrote: >> On 18/04/13 17:00, Martin Kletzander wrote: >>> On 04/18/2013 10:54 AM, Osier Yang wrote: >>>> On 18/04/13 16:42, Martin Kletzander wrote: >>>>> On 04/18/2013 06:36 AM, Laine Stump wrote: >>>>>> The rng schema for <controller> had been non-specific about >>>>>> which >>>>>> types of controllers allowed which models, and also allowed the >>>>>> num_queues attribute (since that hasn't been released yet, >>>>>> should we >>>>>> rename it to "numQueues"?) >>>>> Since there's still time (the commit with that is >>>>> v1.0.4-65-gd4bf0a9), I >>>>> think we should rename it ASAP since we are using camelCase for >>>>> all the >>>>> attribute names. >>>>> >>>>> Apart from that, the RNG with this patch is precise according to >>>>> the >>>>> documentation, so ACK. I'll try to send the numQueues patch >>>>> to see >>>>> what >>>>> others think. >>>> I guess you mean multiple queues support for virtio network? >>>> Regardless of which style we will use finally, FYI, >>>> "num_queues" is >>>> used for disk.. Personally I'm fine with either, because we >>>> already >>>> use both across. >>>> >>> Yes, I meant the virtio-scsi num_queues. As we're trying not to >>> use >>> underscores in XML, I hope we can still switch it. I haven't >>> found any >>> other num_queues anywhere in the code. Could you point me to the >>> commit >>> that uses that? I'm sending the previously discussed patch in the >>> meantime. >>> >> Except the virtio-scsi num_queues, there is no other tag for >> multiple >> queue yet, we will need a patch to support multiple queue for the >> network, >> but it's not committed yet. >> >> It's fine to convert it now, 1.0.5 is not released yet. But is it >> deserved to >> do, we already have many tags with underscore, which can't be >> changed >> for back-compat. >> > I believe those attributes [1] were created by mistake, and kept only > because of backward compatibility. I'm trying to be open-minded, > though, so I'm not forcing my patch in, but seeing it just as a > proposal. Others may have different opinions and I'm willing to > discuss > that. My first feeling, though, was that we should try to keep the > same > policy for as many of them as possible. OTOH, I've mistaken the > underscore with a hyphen when I remembered what Daniel told me about > attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose. But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a
On 04/18/2013 07:27 AM, Osier Yang wrote: particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement. This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...>
And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...>
This is the properties the QEMU device "scsi-disk" supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32
And the properties "virtio-blk-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off
And the properties "virtio-scsi-pci" device supports:
% ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off
We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller.
Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go... Wait. So you're saying that num_queues is a property of the *controller* and not of the individual disk, but you've put the config option in the <disk> rather than the <controller>? Why would you do that? If it's a
On 04/19/2013 04:32 AM, Osier Yang wrote: property of the controller, put the tuning parameter in <controller>. Otherwise, what do you do when one <disk> is configured for num_queues=10 and another disk on the same controller is configured for num_queues=2?
Okay, I misunderstood what you said - you weren't saying that you had put num_queues in the <disk> element (obviously - if I was able to retain enough context in my brain to remember the beginning of the thread, I would have known that :-P), but were instead suggesting that I had meant the num_queues should go in the <driver> subelement of <disk>. You misunderstood me (so we're even :-).
Okay. even misunderstanding indeed :-)
What I was saying was that it should go in the <driver> subelement of <controller>. I still stand by that opinion.
There is no <driver> for <controller> yet. But in case of we already use <driver> in places, if you agreed with introducing one, it means there are 2 votes, and I will do it... Osier
On 04/22/2013 08:00 AM, Laine Stump wrote:
On 04/19/2013 11:25 AM, Laine Stump wrote:
>>>>> On 04/18/2013 06:36 AM, Laine Stump wrote:
Trimming a rather deep nesting...
Okay, I misunderstood what you said - you weren't saying that you had put num_queues in the <disk> element (obviously - if I was able to retain enough context in my brain to remember the beginning of the thread, I would have known that :-P), but were instead suggesting that I had meant the num_queues should go in the <driver> subelement of <disk>. You misunderstood me (so we're even :-). What I was saying was that it should go in the <driver> subelement of <controller>. I still stand by that opinion.
Yes, I think a <driver> subelement of <controller> is a good idea, and like the compromise of 'queues' instead of 'num_queues' as completely avoiding the _ vs. camelCase question (at least for this issue). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote: > On 18/04/13 16:42, Martin Kletzander wrote: >> On 04/18/2013 06:36 AM, Laine Stump wrote: >>> The rng schema for <controller> had been non-specific about which >>> types of controllers allowed which models, and also allowed the >>> num_queues attribute (since that hasn't been released yet, >>> should we >>> rename it to "numQueues"?) >> Since there's still time (the commit with that is >> v1.0.4-65-gd4bf0a9), I >> think we should rename it ASAP since we are using camelCase for >> all the >> attribute names. >> >> Apart from that, the RNG with this patch is precise according to >> the >> documentation, so ACK. I'll try to send the numQueues patch to see >> what >> others think. > I guess you mean multiple queues support for virtio network? > Regardless of which style we will use finally, FYI, "num_queues" is > used for disk.. Personally I'm fine with either, because we already > use both across. > Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...> And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...> This is the properties the QEMU device "scsi-disk" supports: % ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32 And the properties "virtio-blk-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off And the properties "virtio-scsi-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. That means, from libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller. Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go... Osier
On 18/04/13 19:59, Laine Stump wrote:
On 04/18/2013 07:27 AM, Osier Yang wrote:
On 18/04/13 19:16, Laine Stump wrote:
On 04/18/2013 11:05 AM, Osier Yang wrote:
On 18/04/13 17:00, Martin Kletzander wrote:
On 04/18/2013 10:54 AM, Osier Yang wrote: > On 18/04/13 16:42, Martin Kletzander wrote: >> On 04/18/2013 06:36 AM, Laine Stump wrote: >>> The rng schema for <controller> had been non-specific about which >>> types of controllers allowed which models, and also allowed the >>> num_queues attribute (since that hasn't been released yet, >>> should we >>> rename it to "numQueues"?) >> Since there's still time (the commit with that is >> v1.0.4-65-gd4bf0a9), I >> think we should rename it ASAP since we are using camelCase for >> all the >> attribute names. >> >> Apart from that, the RNG with this patch is precise according to >> the >> documentation, so ACK. I'll try to send the numQueues patch to see >> what >> others think. > I guess you mean multiple queues support for virtio network? > Regardless of which style we will use finally, FYI, "num_queues" is > used for disk.. Personally I'm fine with either, because we already > use both across. > Yes, I meant the virtio-scsi num_queues. As we're trying not to use underscores in XML, I hope we can still switch it. I haven't found any other num_queues anywhere in the code. Could you point me to the commit that uses that? I'm sending the previously discussed patch in the meantime.
Except the virtio-scsi num_queues, there is no other tag for multiple queue yet, we will need a patch to support multiple queue for the network, but it's not committed yet.
It's fine to convert it now, 1.0.5 is not released yet. But is it deserved to do, we already have many tags with underscore, which can't be changed for back-compat.
I believe those attributes [1] were created by mistake, and kept only because of backward compatibility. I'm trying to be open-minded, though, so I'm not forcing my patch in, but seeing it just as a proposal. Others may have different opinions and I'm willing to discuss that. My first feeling, though, was that we should try to keep the same policy for as many of them as possible. OTOH, I've mistaken the underscore with a hyphen when I remembered what Daniel told me about attributes [2]. I had recalled DV saying something about underscores in the names a long time ago, and I recently asked about underscore vs. camelCase, and danpb said the same thing. (Personally I don't have a preference one way or
On 04/18/2013 05:41 AM, Martin Kletzander wrote: the other, but if we really are trying to avoid them, now is our chance). I'm fine with either keeping it or changing num_queues. For long term consistence, I agreed with having a consistent naming style is nice.
In the meantime, in other device types, we've tried to keep backend details like this pushed into a <driver> subelement when possible, to avoid polluting the main element (e.g. see the <driver> subelement of <interface>). Is it worth putting this numQueues attribute in a <driver> subelement too? Or am I just playing XML God? Not sure if you mean the upcoming numQueues for interface. But for the existing num_queues, it's for the virtio-scsi controller, putting it in <driver> doesn't reflect the purpose.
But isn't it a backend implementation detail of the specific SCSI controller? In <interface> and <disk>, information that is specific to a particular backend (and isn't generally applicable to that type of device) is in the <driver> subelement.
This is the QEMU command line for a virtio-scsi disk: ("-device virtio-scsi-pci" is mapped to virtio-scsi controller in libvirt XML, with num_queues set): <...> -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ -device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ </...> And this is the QEMU command line for a virtio disk (with event_idx set): <...> -drive file=/var/lib/libvirt/images/f14.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,event_idx=on,scsi=off,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ </...> This is the properties the QEMU device "scsi-disk" supports: % ./x86_64-softmmu/qemu-system-x86_64 -device scsi-disk,? scsi-disk.drive=drive scsi-disk.logical_block_size=blocksize scsi-disk.physical_block_size=blocksize scsi-disk.min_io_size=uint16 scsi-disk.opt_io_size=uint32 scsi-disk.bootindex=int32 scsi-disk.discard_granularity=uint32 scsi-disk.ver=string scsi-disk.serial=string scsi-disk.vendor=string scsi-disk.product=string scsi-disk.removable=on/off scsi-disk.dpofua=on/off scsi-disk.wwn=hex64 scsi-disk.channel=uint32 scsi-disk.scsi-id=uint32 scsi-disk.lun=uint32 And the properties "virtio-blk-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-blk-pci,? virtio-blk-pci.class=hex32 virtio-blk-pci.ioeventfd=on/off virtio-blk-pci.vectors=uint32 virtio-blk-pci.indirect_desc=on/off virtio-blk-pci.event_idx=on/off virtio-blk-pci.drive=drive virtio-blk-pci.logical_block_size=blocksize virtio-blk-pci.physical_block_size=blocksize virtio-blk-pci.min_io_size=uint16 virtio-blk-pci.opt_io_size=uint32 virtio-blk-pci.bootindex=int32 virtio-blk-pci.discard_granularity=uint32 virtio-blk-pci.cyls=uint32 virtio-blk-pci.heads=uint32 virtio-blk-pci.secs=uint32 virtio-blk-pci.serial=string virtio-blk-pci.config-wce=on/off virtio-blk-pci.scsi=on/off virtio-blk-pci.addr=pci-devfn virtio-blk-pci.romfile=string virtio-blk-pci.rombar=uint32 virtio-blk-pci.multifunction=on/off virtio-blk-pci.command_serr_enable=on/off And the properties "virtio-scsi-pci" device supports: % ./x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi-pci,? virtio-scsi-pci.ioeventfd=on/off virtio-scsi-pci.vectors=uint32 virtio-scsi-pci.indirect_desc=on/off virtio-scsi-pci.event_idx=on/off virtio-scsi-pci.hotplug=on/off virtio-scsi-pci.param_change=on/off virtio-scsi-pci.num_queues=uint32 virtio-scsi-pci.max_sectors=uint32 virtio-scsi-pci.cmd_per_lun=uint32 virtio-scsi-pci.addr=pci-devfn virtio-scsi-pci.romfile=string virtio-scsi-pci.rombar=uint32 virtio-scsi-pci.multifunction=on/off virtio-scsi-pci.command_serr_enable=on/off We can put things like "ioeventfd", "event_idx" in the <driver> subelement, is because of the QEMU device used for disk supports it. But for a virtio-scsi disk, "num_queues" is supported in a separate device "virtio-scsi-pci" instead.. From libvirt p.o.v, things like "ioevent_idx" are for disk, "num_queues" is for the disk controller. Assuming that we put "num_queues" or "numQueues" in <driver>, then we need to find out the controller for disk when building QEMU command line, and check if it's virtio-scsi model, if not, error out, otherwise tell the function to build the controller device string that "num_queues" is specified, and what its value is. Or something similar but reversely (find out the disk associated with the virtio-scsi controller, check if num_queues is specified). This might be not the exact process, but it can show putting "num_queues" in <driver> is just a straight wrong way to go... Osier
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Martin Kletzander -
Osier Yang