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

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?