qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, "Michael S . Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH RFC 8/9] KVM: Add dirty-ring-size property
Date: Wed, 25 Mar 2020 16:42:14 -0400	[thread overview]
Message-ID: <20200325204214.GD404034@xz-x1> (raw)
In-Reply-To: <20200325200031.GG2635@work-vm>

On Wed, Mar 25, 2020 at 08:00:31PM +0000, Dr. David Alan Gilbert wrote:
> > @@ -2077,6 +2079,33 @@ static int kvm_init(MachineState *ms)
> >      s->memory_listener.listener.coalesced_io_add = kvm_coalesce_mmio_region;
> >      s->memory_listener.listener.coalesced_io_del = kvm_uncoalesce_mmio_region;
> >  
> > +    /*
> > +     * Enable KVM dirty ring if supported, otherwise fall back to
> > +     * dirty logging mode
> > +     */
> > +    if (s->kvm_dirty_ring_size > 0) {
> > +        /* Read the max supported pages */
> > +        ret = kvm_vm_check_extension(kvm_state, KVM_CAP_DIRTY_LOG_RING);
> > +        if (ret > 0) {
> > +            if (s->kvm_dirty_ring_size > ret) {
> > +                error_report("KVM dirty ring size %d too big (maximum is %d). "
> > +                             "Please use a smaller value.",
> > +                             s->kvm_dirty_ring_size, ret);
> > +                goto err;
> > +            }
> > +
> > +            ret = kvm_vm_enable_cap(s, KVM_CAP_DIRTY_LOG_RING, 0,
> > +                                    s->kvm_dirty_ring_size);
> > +            if (ret) {
> > +                error_report("Enabling of KVM dirty ring failed: %d", ret);
> > +                goto err;
> > +            }
> > +
> > +            s->kvm_dirty_gfn_count =
> > +                s->kvm_dirty_ring_size / sizeof(struct kvm_dirty_gfn);
> 
> What happens if I was to pass dirty-ring-size=1 ?
> Then the count would be 0 and things would get upset somewhere?
> Do you need to check for a minimum postive value?

The above kvm_vm_enable_cap() should fail directly and QEMU will stop.
Yes we should check it, but kernel will do that in all cases, so I
just didn't do that explicitly again in the userspace.

I was planning this to be an advanced feature so the user of this
parameter should know the rules to pass values in.

Of course we can introduce another global knob to enable/disable this
feature as a whole, then I can offer a default value for the size
parameter.  I just didn't bother that in this RFC version, but I can
switch to that if that's preferred.

[...]

> > @@ -3065,6 +3123,12 @@ static void kvm_accel_class_init(ObjectClass *oc, void *data)
> >          NULL, NULL, &error_abort);
> >      object_class_property_set_description(oc, "kvm-shadow-mem",
> >          "KVM shadow MMU size", &error_abort);
> > +
> > +    object_class_property_add(oc, "dirty-ring-size", "int",
> > +        kvm_get_dirty_ring_size, kvm_set_dirty_ring_size,
> > +        NULL, NULL, &error_abort);
> 
> I don't think someone passing in a non-number should cause an abort;
> it should exit, but I don't think it should abort/core.

It won't - &error_abort is for the operation to add the property.  It
should never fail.

User illegal input will look like this (a graceful exit):

$ ./x86_64-softmmu/qemu-system-x86_64 -accel kvm,dirty-ring-size=a
qemu-system-x86_64: -accel kvm,dirty-ring-size=a: Parameter 'dirty-ring-size' expects int64

> 
> > +    object_class_property_set_description(oc, "dirty-ring-size",
> > +        "KVM dirty ring size (<=0 to disable)", &error_abort);
> >  }
> >  
> >  static const TypeInfo kvm_accel_type = {
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 224a8e8712..140bd38726 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -119,6 +119,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
> >      "                kernel-irqchip=on|off|split controls accelerated irqchip support (default=on)\n"
> >      "                kvm-shadow-mem=size of KVM shadow MMU in bytes\n"
> >      "                tb-size=n (TCG translation block cache size)\n"
> > +    "                dirty-ring-size=n (KVM dirty ring size in Bytes)\n"
> >      "                thread=single|multi (enable multi-threaded TCG)\n", QEMU_ARCH_ALL)
> >  STEXI
> >  @item -accel @var{name}[,prop=@var{value}[,...]]
> > @@ -140,6 +141,8 @@ irqchip completely is not recommended except for debugging purposes.
> >  Defines the size of the KVM shadow MMU.
> >  @item tb-size=@var{n}
> >  Controls the size (in MiB) of the TCG translation block cache.
> > +@item dirty-ring-size=@val{n}
> > +Controls the size (in Bytes) of KVM dirty ring (<=0 to disable).
> 
> I don't see the point in allowing < 0 ; I'd ban it.

Sure; I can switch to an uint64.

Thanks,

-- 
Peter Xu



  reply	other threads:[~2020-03-25 20:43 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-05 14:17 [PATCH RFC 0/9] KVM: Dirty ring support (QEMU part) Peter Xu
2020-02-05 14:17 ` [PATCH RFC 1/9] KVM: Fixup kvm_log_clear_one_slot() ioctl return check Peter Xu
2020-03-25 16:34   ` Dr. David Alan Gilbert
2020-03-25 17:43   ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 2/9] linux-headers: Update Peter Xu
2020-02-05 14:17 ` [PATCH RFC 3/9] memory: Introduce log_sync_global() to memory listener Peter Xu
2020-03-25 16:52   ` Dr. David Alan Gilbert
2020-03-25 17:02     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 4/9] KVM: Create the KVMSlot dirty bitmap on flag changes Peter Xu
2020-03-25 17:44   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 5/9] KVM: Provide helper to get kvm dirty log Peter Xu
2020-03-25 17:52   ` Dr. David Alan Gilbert
2020-03-25 18:15     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 6/9] KVM: Provide helper to sync dirty bitmap from slot to ramblock Peter Xu
2020-03-25 18:47   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 7/9] KVM: Cache kvm slot dirty bitmap size Peter Xu
2020-03-25 18:49   ` Dr. David Alan Gilbert
2020-02-05 14:17 ` [PATCH RFC 8/9] KVM: Add dirty-ring-size property Peter Xu
2020-03-25 20:00   ` Dr. David Alan Gilbert
2020-03-25 20:42     ` Peter Xu [this message]
2020-03-26 13:41       ` Dr. David Alan Gilbert
2020-03-26 16:03         ` Peter Xu
2020-03-25 20:14   ` Dr. David Alan Gilbert
2020-03-25 20:48     ` Peter Xu
2020-02-05 14:17 ` [PATCH RFC 9/9] KVM: Dirty ring support Peter Xu
2020-03-25 20:41   ` Dr. David Alan Gilbert
2020-03-25 21:32     ` Peter Xu
2020-03-26 14:14       ` Dr. David Alan Gilbert
2020-03-26 16:10         ` Peter Xu
2020-02-05 14:51 ` [PATCH RFC 0/9] KVM: Dirty ring support (QEMU part) no-reply
2020-03-03 17:32 ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200325204214.GD404034@xz-x1 \
    --to=peterx@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).