Re: [PATCH 2/2] qemu: support dirty ring feature

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jun 19, 2021 at 17:53:04 +0800, huangy81@xxxxxxxxxxxxxxx wrote:
> From: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
> 
> QEMU has introduced a dirty ring feature, this patch add corresponding
> feature named 'dirty-ring', which enable dirty ring feature when
> starting vm.
> 
> To enable the feature, libvirt add "-accel dirty-ring-size=xxx"
> to QEMU command line, the following XML needs to be added to
> the guest's domain description:
> 
> <features>
>    <kvm>
>      <dirty-ring state='on' size='xxx'>
>    </kvm>
> </features>
> 
> If property "state=on" but property "size" not be configured, set
> default ring size with 4096.
> 
> Since dirty ring can only be enabled by specifying "-accel" option
> and do not support the legacy style, it seems that there's no
> other way to work around this, so we use "-accel" option to specify
> accelerator instead of "-machine" when building qemu commandline.
> 
> Details about the qemu "-accel" option:
> https://lore.kernel.org/qemu-devel/3aa73987-40e8-3619-0723-9f17f73850bd@xxxxxxxxxx/
> 
> Three things this patch has done:

Note we require that patches are kept small and self contained. You
describe 3 things this patch is doing ant thus it almost implies as
it could be split 3-ways ...

> 1. switch the option "-machine accel" to "-accel" when specifying
> accelerator type once libvirt build QEMU command line.

this definitely belongs to a separate patch.

> 
> 2. Since this patch change the output of building commandline, qemuxml2argvtest
> should also been modified so that test cases can be executed
> successfully, we modify test cases involved from qemu-2.9.0 to

Note that the oldest supported qemu is now qemu-2.11

> qemu-6.1.0
> 
> 3. Introduce dirty_ring_size in struct "_virDomainDef" to hold the
> ring size configured by user, and pass dirty_ring_size when building qemu
> commandline if dirty ring feature enabled.

This requires at least a 2 way split.
1) patch adding the definition, docs and XML parser/formatter
2) actual qemu impl.

> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@xxxxxxxxxxxxxxx>
> ---

[...]

> @@ -17574,6 +17575,9 @@ virDomainFeaturesDefParse(virDomainDef *def,
>      if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) {
>          int feature;
>          virTristateSwitch value;
> +        xmlNodePtr node = NULL;
> +
> +        node = ctxt->node;
>          if ((n = virXPathNodeSet("./features/kvm/*", ctxt, &nodes)) < 0)
>              return -1;
>  
> @@ -17590,12 +17594,40 @@ virDomainFeaturesDefParse(virDomainDef *def,
>                  case VIR_DOMAIN_KVM_HIDDEN:
>                  case VIR_DOMAIN_KVM_DEDICATED:
>                  case VIR_DOMAIN_KVM_POLLCONTROL:
> +                case VIR_DOMAIN_KVM_DIRTY_RING:

Don't integrate this with other cases ...

>                      if (virXMLPropTristateSwitch(nodes[i], "state",
>                                                   VIR_XML_PROP_REQUIRED,
>                                                   &value) < 0)
>                          return -1;


Preferably rather duplicate this parsing ....



>  
>                      def->kvm_features[feature] = value;
> +
> +                    /* only for dirty ring case */
> +                    if (((virDomainKVM) feature) == VIR_DOMAIN_KVM_DIRTY_RING &&
> +                          value == VIR_TRISTATE_SWITCH_ON) {

To avoid this condition.

> +                        ctxt->node = nodes[i];
> +
> +                        if (virXMLPropString(ctxt->node, "size")) {
> +                            if (virXPathUInt("string(./@size)", ctxt,
> +                                             &def->dirty_ring_size) < 0) {
> +                                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                              _("invalid number of dirty ring size"));
> +                                return -1;
> +                            }
> +
> +                            if ((def->dirty_ring_size & (def->dirty_ring_size - 1)) ||

Zero checks on integers should be explicit.

> +                                def->dirty_ring_size < 1024 ||
> +                                def->dirty_ring_size > 65536) {
> +                                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                               _("dirty ring must be power of 2 "
> +                                                 "and ranges [1024, 65536]"));
> +                                return -1;
> +                            }
> +                        } else {
> +                            /* ring size not configured, set with default value 4096 */
> +                            def->dirty_ring_size = 4096;

Assigning defaults in the parser isn't current state of things.
Preferably this belongs to a post-parse callback.

> +                        }
> +                    }
>                      break;
>  
>                  case VIR_DOMAIN_KVM_LAST:

[...]

> @@ -21627,6 +21660,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
>              case VIR_DOMAIN_KVM_HIDDEN:
>              case VIR_DOMAIN_KVM_DEDICATED:
>              case VIR_DOMAIN_KVM_POLLCONTROL:
> +            case VIR_DOMAIN_KVM_DIRTY_RING:
>                  if (src->kvm_features[i] != dst->kvm_features[i]) {
>                      virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                     _("State of KVM feature '%s' differs: "

The abi-stability check should also check the size.

> @@ -27614,6 +27648,15 @@ virDomainDefFormatFeatures(virBuffer *buf,
>                                                def->kvm_features[j]));
>                      break;
>  
> +                case VIR_DOMAIN_KVM_DIRTY_RING:
> +                    if (def->kvm_features[j])

Make the comparison with VIR_TRISTATE_SWITCH_ABSENT explicit here.

> +                        virBufferAsprintf(&childBuf, "<%s state='%s' size='%d'/>\n",
> +                                          virDomainKVMTypeToString(j),
> +                                          virTristateSwitchTypeToString(
> +                                          def->kvm_features[j]),

Don't linebreak the argument to virTristateSwitchTypeToString even if it
exceeds the line lenght. It's unreadable this way.

> +                                          def->dirty_ring_size);
> +                    break;
> +
>                  case VIR_DOMAIN_KVM_LAST:
>                      break;
>                  }

[...]


> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ea51369..80142b2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6587,6 +6587,9 @@ qemuBuildCpuCommandLine(virCommand *cmd,
>                      virBufferAddLit(&buf, ",kvm-poll-control=on");
>                  break;
>  
> +            case VIR_DOMAIN_KVM_DIRTY_RING:
> +                break;
> +
>              case VIR_DOMAIN_KVM_LAST:
>                  break;
>              }
> @@ -6758,29 +6761,41 @@ qemuBuildNameCommandLine(virCommand *cmd,
>      return 0;
>  }
>  
> +static void
> +qemuBuildAccelCommandLineTcgOptions(void)
> +{
> +    /* TODO: build command line tcg accelerator
> +     * property like tb-size */

Is this patch incomplete? In case you don't want to support this for TCG
you should forbid it in validation rather than silently ignoring it.

> +}
> +
> +static void
> +qemuBuildAccelCommandLineKvmOptions(virBuffer *buf,
> +                                    const virDomainDef *def)
> +{
> +    if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON &&
> +        def->kvm_features[VIR_DOMAIN_KVM_DIRTY_RING] == VIR_TRISTATE_SWITCH_ON) {
> +        virBufferAsprintf(buf, ",dirty-ring-size=%d", def->dirty_ring_size);
> +    }
> +}
> +
>  static int
> -qemuBuildMachineCommandLine(virCommand *cmd,
> -                            virQEMUDriverConfig *cfg,
> -                            const virDomainDef *def,
> -                            virQEMUCaps *qemuCaps,
> -                            qemuDomainObjPrivate *priv)
> +qemuBuildAccelCommandLine(virCommand *cmd,
> +                          const virDomainDef *def)
>  {
> -    virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
> -    virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
> -    virCPUDef *cpu = def->cpu;
> +    /* the '-machine' options for accelerator are legacy,
> +     * using the '-accel' options by default */
>      g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> -    size_t i;
> -
> -    virCommandAddArg(cmd, "-machine");
> -    virBufferAdd(&buf, def->os.machine, -1);
> +    virCommandAddArg(cmd, "-accel");

This MUST be a separate commit it's a fully separate change which
requires separate justification.

>  
>      switch ((virDomainVirtType)def->virtType) {
>      case VIR_DOMAIN_VIRT_QEMU:
> -        virBufferAddLit(&buf, ",accel=tcg");
> +        virBufferAddLit(&buf, "tcg");
> +        qemuBuildAccelCommandLineTcgOptions();
>          break;
>  
>      case VIR_DOMAIN_VIRT_KVM:
> -        virBufferAddLit(&buf, ",accel=kvm");
> +        virBufferAddLit(&buf, "kvm");
> +        qemuBuildAccelCommandLineKvmOptions(&buf, def);
>          break;
>  
>      case VIR_DOMAIN_VIRT_KQEMU:
> @@ -6808,6 +6823,61 @@ qemuBuildMachineCommandLine(virCommand *cmd,
>          return -1;
>      }
>  
> +    virCommandAddArgBuffer(cmd, &buf);
> +    return 0;
> +}
> +
> +static int
> +qemuBuildMachineCommandLine(virCommand *cmd,
> +                            virQEMUDriverConfig *cfg,
> +                            const virDomainDef *def,
> +                            virQEMUCaps *qemuCaps,
> +                            qemuDomainObjPrivate *priv)
> +{
> +    virTristateSwitch vmport = def->features[VIR_DOMAIN_FEATURE_VMPORT];
> +    virTristateSwitch smm = def->features[VIR_DOMAIN_FEATURE_SMM];
> +    virCPUDef *cpu = def->cpu;
> +    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> +    size_t i;
> +
> +    virCommandAddArg(cmd, "-machine");
> +    virBufferAdd(&buf, def->os.machine, -1);
> +
> +    /* QEMU lower than 2.9.0 do not support '-accel'
> +     * fallback to '-machine accel' */

We don't support those qemu versions. This is the reason why you must
split the patch. A refactor like this has nothing to do with the feature
addition and should be justified and reviewed separately.

> +    if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
> +        switch ((virDomainVirtType)def->virtType) {
> +        case VIR_DOMAIN_VIRT_QEMU:
> +            virBufferAddLit(&buf, ",accel=tcg");
> +            break;
> +        case VIR_DOMAIN_VIRT_KVM:
> +            virBufferAddLit(&buf, ",accel=kvm");
> +            break;
> +        case VIR_DOMAIN_VIRT_KQEMU:


[...]

> @@ -10402,6 +10472,11 @@ qemuBuildCommandLine(virQEMUDriver *driver,
>      if (qemuBuildMachineCommandLine(cmd, cfg, def, qemuCaps, priv) < 0)
>          return NULL;
>  
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_ACCEL)) {
> +        if (qemuBuildAccelCommandLine(cmd, def) < 0)
> +            return NULL;
> +    }
> +
>      qemuBuildTSEGCommandLine(cmd, def);
>  
>      if (qemuBuildCpuCommandLine(cmd, driver, def, qemuCaps) < 0)




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux