[libvirt] [PATCH] rng: tighten up domain <controller> schema

Osier Yang jyang at redhat.com
Mon Apr 22 15:16:58 UTC 2013


On 22/04/13 22:00, Laine Stump wrote:
> On 04/19/2013 11:25 AM, Laine Stump wrote:
>> 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 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
>>>> 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?
>>
> 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




More information about the libvir-list mailing list