Re: [Qemu-devel] [PULL v4 07/11] rdma: introduce capability for chunk registration

[Michael S. Tsirkin]

On Thu, Apr 18, 2013 at 04:07:24PM -0600, Eric Blake wrote:
> On 04/17/2013 05:07 PM, address@hidden wrote:
> > From: "Michael R. Hines" <address@hidden>
> > 
> > This capability allows you to disable dynamic chunk registration
> > for better throughput on high-performance links.
> > 
> > It is enabled by default.
> > 
> > Signed-off-by: Michael R. Hines <address@hidden>
> > ---
> >  migration.c      |   10 ++++++++++
> >  qapi-schema.json |    8 +++++++-
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> >  #
> > +# @x-chunk-register-destination: (since 1.5) RDMA option which controls 
> > whether
> > +#          or not the entire VM memory footprint is mlock() on demand or 
> > all at once.
> > +#          Refer to docs/rdma.txt for more advice on when to take 
> > advantage option.
> 
> s/take advantage/use this/
> 
> > +#          Enabled by default, and will be renamed to 
> > 'chunk-register-destination' 
> > +#          after experimental testing is complete.
> 
> I wouldn't promise a rename - after all, testing may prove that we can
> settle on enough heuristics to set this appropriately without needing a
> user option, even for the workloads where it makes a difference.  Thus,
> I think better wording might be:
> 
> Enabled by default.  Experimental: may be renamed or removed after
> further testing is complete.
> 
> Sorry for not thinking about this earlier, but typically you want a
> capability bit to default to 0 - it's much easier to assume that a bit
> not present behaves the same as a bit that is present and 0.  Or put
> another way, a older management app that asks for all enabled
> capabilities on a newer qemu has an easier time ignoring 0 bits that it
> doesn't recognize (oh, some new feature I don't know about, but it isn't
> on, so it can't hurt) than it does ignoring 1 bits (oh, a feature I
> don't recognize, but it's enabled - will it mess up my migration?).
> Since this is a bool, I would much rather can we rename the capability
> to express the opposite sense, and default it to 0.  I'm not even sure
> from your description here whether 'true' means 'mlock() on demand' or
> 'all at once', just that I'm supposed to read rdma.txt to decide if I
> want to move away from the default.
> 
> /me reads patch 11 again... and wonders why the docs came last instead
> of first in the series...
> 
> I guess the opposite sense could be named 'x-rdma-pin-all'; default
> false means to do chunk registration and release,

chunk release only happens after migration is complete unfortunately.
This means that eventually all initialized memory is pinned, regardless
of the setting (this is fixable but there's no plan to fix this, at this
point). So pin-all might be misleading to some.

I agree 'chunk' is unnecessarily low level though.
The only difference ATM is pinning of uninitialized memory so I think a
better name would be 'x-rdma-pin-uninitialized' or some such.

> true means to pin all
> memory up front.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>