Re: [libvirt] [PATCH 5/5] qemu: Add support for setting the TSEG size

On 05/31/2018 03:24 AM, Martin Kletzander wrote: > On Wed, May 30, 2018 at 04:00:32PM -0400, John Ferlan wrote: >> >> This is way too sparse. >> > > I can't really think of what to say here that is not already mentioned > in the > documentation or self-explanatory in the code.  But maybe only I see > that as > self-explanatory.  I'll write something up here. > The whole default thing is what really drew my attention back to the top... It's where I expected to see an explanation why the default was being set. I think that's something that should be described - you may disagree, but that's why it's a review. [...] See, I know how to cut - I just forgot in my response to patch 4 ;-) I'll try not to overcut as I've seen done in for reviews I've received which means I have to find the context of the feedback (we all have our favorite things to gripe about). >>> + >>> +static void >>> +qemuBuildTSEGCommandLine(virCommandPtr cmd, >>> +                         const virDomainDef *def) >>> +{ >>> +    if (!def->tseg_size) >> >> Since it's not bool or pointer, prefer the tseg_size == 0 >> > > I don't know if you mean that as indicative or imperative.  It is very > subjective and for such scenarios I would like to execute my right to > mention > that it is not part of Contributor Guidelines.  I know it might seem > rude, but > this is one of the things that's very subjective and for such things I > like to > first reach a consensus before I change what I'm used to writing.  I > hope you > understand that. > There's a lot of things not specifically listed in the CG that are mentioned in many reviews or that are just now done because they have been mentioned (2 blank lines between functions, formatting of function headers, 1 variable per line, etc.). When I see this style and remember to note it, I do... And I have see others mention it too. Many times it's for enum comparisons.

In the long run, whatever. It's a style preference that denotes variable usage for those reading code "in the future". We don't have a rule for it, so go with your style if that's what you prefer. FWIW: back in the dark ages when I first started doing this... Something like the above for an integer, long, etc. value would be converted by the compiler to checking *only* if the low bit was set/cleared (think little endian) because that instruction was really quick. Try to find that needle in a haystack for each even value that could be used ;-). I got very used to the explicit comparison to 0 just to be 100% clear. >>> +        return; >>> + >>> +    virCommandAddArg(cmd, "-global"); >>> + [...] >>> +    /* QEMU started defaulting to 16MiB after 2.9 */ >>> +    if (major > 2 || (major == 2 && minor > 9)) >>> +        def->tseg_size = 16 * 1024 * 1024; >> >> So, if QEMU defaults to 16MiB, then why do we need so set this at all? >> > > TL;DR because not setting the default value when it is not explicitly > requested has already bitten us in the back multiple times. > > Long story short this way we are safe.  No matter what happens (QEMU > changing the default, the user changing the config somewhere else and > not expecting this to change, us wanting to change the default in the > future for some reason, migration to newer libvirt where who-knows-what > is checked there, etc.) it is way easier to keep the guest ABI stable. > It is visible what the value actually is and we're not hiding it > somewhere in the code of the hypervisor.  I know we don't do that for > many things.  I could've gone with the (often alibistic [1]) "the > default is left for hypervisor to decide", but since we have the > opportunity tomake things right (thanks to Laszlo's explanations), why > shouldn't we? > >> This seems to me we are setting policy which based on history of many >> patches before is a no go.  I'd say NAK to this, unless there is some >> convincing argument made in the commit message and followup responses to >> the series (or you can take Jan's R-By and ignore me - your call. >> > > What patches are you referring to?  I can add that to the commit > message, sure. > In my mind, for starters what I mentioned in my response to patch 3 for formatdomain.html.in changes. Beyond that, I don't really don't have the patience to go through hundreds of patches of history in order to pick out ones that were NACK'd or requested to be changed because the patch made some policy decision or a let's set a default value type decision. I know they've happened though. TL;DR: I'm still of the opinion we don't set (or even save) a default.

I would think someone that failed to boot their guest because the memory size or vCPU count was too large for smm/tseg would then be pointed to this value which they would then optionally add and then "manage" going forward. I still don't see the point of setting a default if the default for QEMU has been good enough up to now up to and including a certain memory size of number of vCPU's, then how does us setting that default change anything? Especially if it's the same value that QEMU uses. How do we know that the value in the XML is one we provided or one the user provided? Without providing a default and if QEMU changed the default would a migration fail? If we set a default and then the migration failed, where does the fickle finger of fate fall? Yes, Laszlo's explanations are great, but they're lost to the mailing list and the fading memories of those reading them if not captured in some way in comments to code. If there's a consensus to allow a default to be listed, then I'm not going to even try to block. However, not mentioning it or calling it out as part of the review would to me would be wrong. I believe by doing this you're setting a precedent although I could be wrong as I don't have the entire code base memorized.

>>> + >>> +    return 0; >>> + >>> + error: >>> +    virReportError(VIR_ERR_INTERNAL_ERROR, >>> +                   _("Cannot parse QEMU machine type version '%s'"), >>> +                   def->os.machine); >>> +    return -1; >>> +} >>> + >>> + >>> +static int >>> +qemuDomainDefTsegPostParse(virDomainDefPtr def, >> >> While TSEG is what you're adjusting, this is more a 'features' or in >> particular SMM features post parse callback. >> > > I'm getting the hint of a hidden suggestion from the sentence.  Are you > suggesting I should rename the function or create few more layers like > this? > > qemuDomainDefSMMFeaturesPostParse() { >    qemuDomainDefTsegPostParse(); > } > > qemuDomainDefFeaturesPostParse() { >    qemuDomainDefSMMFeaturesPostParse(); > } > > qemuDomainDefPostParse() { >    ... >    qemuDomainDefFeaturesPostParse(); >    ... > } > While the above is the progression I had in mind, I cannot make my mind up if we should go through the insanity of all that or just go with the qemuDomainDefSMMFeaturesPostParse which "allows" someone with a future need to create a qemuDomainDefXXXFeaturesPostParse that is then either directly called from qemuDomainDefPostParse or indirectly from a new qemuDomainDefFeaturesPostParse. This another one of those things not explicitly stated in the CG that I know I've received feedback on in the past or have seen done by other patches with respect to the naming of functions and/or creation of shell methods to drill down into the path of the details. Function naming can be so subjective... >>> +                           virQEMUCapsPtr qemuCaps) >>> +{ >>> +    if (def->features[VIR_DOMAIN_FEATURE_SMM] != >>> VIR_TRISTATE_SWITCH_ON) >>> +        return 0; >>> + >>> +    if (!def->tseg_size) >> >> Similar... prefer tseg_size == 0 >> > > Again, are you directing me to do that or saying what version you like > more? > Pointing out a similar construct that IMO should be changed, but you seem to dislike that way of comparison, so whatever. Yes, I know it's functionally equivalent. >>> +        return qemuDomainSetDefaultTsegSize(def, qemuCaps); >>> + >>> +    if (!qemuDomainIsQ35(def)) { >>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>> +                       _("SMM TSEG is only supported with q35 >>> machine type")); >>> +        return -1; >>> +    } >>> + >>> +    if (!virQEMUCapsGet(qemuCaps, >>> QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES)) { >>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>> +                       _("Setting TSEG size is not supported with >>> this QEMU binary")); >>> +        return -1; >>> +    } >>> + >>> +    if (VIR_ROUND_UP(def->tseg_size, 1024 * 1024) != def->tseg_size) { >>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >>> +                       _("SMM TSEG size must be divisible by 1 MiB")); >>> +        return -1; >>> +    } >> >> Does anywhere else that does a VIR_ROUND_UP elicit an error?  Why bother. >> >> Curious that this differs from qemuDomainMemoryDeviceAlignSize which >> claims 1MiB rounding using qemuDomainGetMemorySizeAlignment which >> returns 1024 unless it's PPC64 which returns 256MiB alignments. > > It does.  The extended TSEG explicitly allows 1 MiB increments.  Not > only because of the naming (*_mbytes), but also because bigger > granularity would just be pointless probably.  Also this is not going to > exist on PPC64, it only makes sense with q35. > I was partially commenting on why even bother - just force the rounding and indicate that libvirt will do so - that I think has been done numerous times before. Failing just because someone (tester) used 12345KiB or 1025KiB just seems pointless especially if we can and say we will round if you don't know how to make things 1MiB aligned. A value below 1 MiB would be useless - in fact it seems from the default discussion that anything below 16 MiB would be useless, wouldn't it? As for the rest - I went and checked a few other VIR_ROUND_UP consumers and they were claiming the 1MiB rounding, but using 1024 instead of 1024*1024 like is done above which I just found "curious" - I didn't bother to check the basis of the value being stored, but I think for memory it is KB.

>>> + >>> +    return 0; >>> +} >>> + >>> + [...] >>> +++ b/tests/qemuxml2argvdata/tseg-explicit-size.xml >>> @@ -0,0 +1,23 @@ >>> +<domain type='qemu'> >>> +  <name>QEMUGuest1</name> >>> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >>> +  <memory unit='KiB'>219100</memory> >>> +  <currentMemory unit='KiB'>219100</currentMemory> >>> +  <vcpu placement='static'>1</vcpu> >>> +  <os> >>> +    <type arch='x86_64' machine='pc-q35-2.10'>hvm</type> >>> +    <boot dev='hd'/> >>> +  </os> >>> +  <features> >>> +    <smm state='on'> >>> +      <tseg>48</tseg> >> >> The only difference here from genericxml2xmlindata/tseg.xml is that this >> one doesn't have "unit='MiB'" >> > > Yes, I wanted to check for the correct parsing of both.  Maybe I should > put 1 GiB in the other one so that I check that it parses the unit > properly?  Actually I have KiB somewhere, so probably not. > Considering you commented recently on another series regarding whether the code should use genericxml2xml - I'm just pointing out the IMO excess of having both generic and qemu tests doing pretty much the same thing... You can do what you want though - if both are left there, that's fine.

>>> +    </smm> >>> +  </features> >>> +  <clock offset='utc'/> >>> +  <on_poweroff>destroy</on_poweroff> >>> +  <on_reboot>restart</on_reboot> >>> +  <on_crash>destroy</on_crash> >>> +  <devices> >>> +    <emulator>/usr/bin/qemu-system-x86_64</emulator> >>> +  </devices> >>> +</domain> [...] >>> --- /dev/null >>> +++ b/tests/qemuxml2argvdata/tseg.xml >> >> and yet another one similr to genericxml2xmlindata/tseg.xml - same name, >> different directory >> > > Are you suggesting I create this as a symlink?  Or that I should add > info to the commit message that this patch adds checks for formatting > the command-line (the functionality the patch is adding) and that the > genericxml2xmltest is checking the parsing even for developers who build > with QEMU driver disabled? > IMO: More duplication of the same test... I'm not the first person to ever point out duplication of tests in commits. "Some day" maybe qemuxml2xmltest will be able to source from genericxml2xml - then it's up to someone to decide which test to use.

[...] >>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >>> index 78454acb1a41..633dfaaee9a4 100644 >>> --- a/tests/qemuxml2argvtest.c >>> +++ b/tests/qemuxml2argvtest.c >>> @@ -2827,6 +2827,54 @@ mymain(void) >>> [...] >>> +    DO_TEST_PARSE_ERROR("tseg-i440fx", >>> +                        QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, >>> +                        QEMU_CAPS_DEVICE_PCI_BRIDGE, >>> +                        QEMU_CAPS_DEVICE_IOH3420, >>> +                        QEMU_CAPS_ICH9_AHCI, >>> +                        QEMU_CAPS_MACHINE_SMM_OPT, >>> +                        QEMU_CAPS_VIRTIO_SCSI, >>> +                        QEMU_CAPS_MCH_EXTENDED_TSEG_MBYTES); >> Failure wrong machine type... >> >>> +    DO_TEST_PARSE_ERROR("tseg-explicit-size", >>> +                        QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, >>> +                        QEMU_CAPS_DEVICE_PCI_BRIDGE, >>> +                        QEMU_CAPS_DEVICE_IOH3420, >>> +                        QEMU_CAPS_ICH9_AHCI, >>> +                        QEMU_CAPS_MACHINE_SMM_OPT, >>> +                        QEMU_CAPS_VIRTIO_SCSI); >> >> Failure because TSEG_MBYTES not provided. >> > > I'm sorry, don't take this personally, but sometimes I'm not sure > whether you are trying to convey some information or you are just adding > comments for yourself.  If you are asking me to do something, then > please say so.  If you want these messages to be there as comments, let > me know, I'll do that (although the only time that would be useful is if > someone makes a change and the tests start failing (by the tested code > not failing) and that person wants to know what was supposed to be the > error.  In which case they can re-run them without the modifications. > Or if you are suggesting that it should be DO_TEST_FAILURE instead of > DO_TEST_PARSE_ERROR, then it cannot be, the tests would fail because the > code doesn't throw an error during starting the domain, but right away > during parsing.  And yes, that can be done because I'm not erroring out > for something that existed before, but for a new feature so it will > never be read from the disk for a domain that existed before this > series.  Or maybe I ran the tests wrong and it is supposed to be > DO_TEST_FAILURE and I made a mistake somewhere else as well. > > The reason for me not really being sure what you mean by that is that I > take all of what's in the previous paragraph as something obvious and > known by default for non-newbie libvirt developers (which you most > certainly are one since you have years of experience in this codebase), > so I'm probably just missing something.  It very well might be a > language barrier, because I'm nowhere near understanding most of the > nuances in English language and various dialects thereof.  So with no > hidden meaning between the lines, please enlighten me. > Nothing personal... and valid question/points. There's no hidden meaning - sometimes it's just notes for myself that I forget to go back and remove and other times it's just validation of what's being tested. Part of my review processing is going from the test to the code and considering the error being tested. Just like some people forget to remove debugging logic from their patches before posting, I just forgot to remove my review debugging notes - mea culpa and apologies for any added stress/anxiety ;-). Since Jan had already provided an R-By for the patch - I figured anything I'm providing becomes feedback for you to pick and choose which you find particularly necessary to incorporate and which to ignore.

After a few years of getting reviews I've learned the styles of various reviewers and what feedback can be ignored, heeded, and challenged. You've been working on libvirt longer than me, so I'm sure you have certain filters set already too.

John [...]