Re: [Xen-devel] [PATCHv6 3/3] xen/privcmd: improve performance of MMAPBATCH_V2

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 9 Mar 2015, David Vrabel wrote:
> On 09/03/15 12:25, Stefano Stabellini wrote:
> > On Fri, 6 Mar 2015, David Vrabel wrote:
> >> Make the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version) map
> >> multiple frames at a time rather than one at a time, despite the pages
> >> being non-consecutive GFNs.
> >>
> >> xen_remap_foreign_mfn_array() is added which maps an array of GFNs
> >> (instead of a consecutive range of GFNs).
> >>
> >> Migrate times are significantly improved (when using a PV toolstack
> >> domain).  For example, for an idle 12 GiB PV guest:
> >>
> >>         Before     After
> >>   real  0m38.179s  0m26.868s
> >>   user  0m15.096s  0m13.652s
> >>   sys   0m28.988s  0m18.732s
> >>
> >> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> > 
> > Nice!
> 
> Note that is is for PV toolstack domains only.  Someone else would need
> to implement the hypercall batching for auto-xlated physmap guests.
> 
> >> +/* Not used in ARM. Use xen_remap_domain_mfn_array(). */
> >> +int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
> >> +                              unsigned long addr,
> >> +                              xen_pfn_t mfn, int nr,
> >> +                              pgprot_t prot, unsigned domid,
> >> +                              struct page **pages)
> >> +{
> >> +  return -ENOSYS;
> >> +}
> >>  EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range);
> > 
> > I understand that it is not used, but since you have already introduced
> > xen_remap_domain_mfn_array, you might as well implement
> > xen_remap_domain_mfn_range by calling xen_remap_domain_mfn_array or
> > xen_xlate_remap_gfn_array.
> 
> I should I spend time implementing something that isn't used?

In that case, please change the comment to:

/* Not used by XENFEAT_auto_translated guests. */


> >> +                  ret = __get_user(mfn, st->user_mfn);
> >> +                  if (ret < 0)
> >> +                          return ret;
> >> +                  /*
> >> +                   * V1 encodes the error codes in the 32bit top
> >> +                   * nibble of the mfn (with its known
> >> +                   * limitations vis-a-vis 64 bit callers).
> >> +                   */
> >> +                  mfn |= (ret == -ENOENT) ?
> >> +                          PRIVCMD_MMAPBATCH_PAGED_ERROR :
> >> +                          PRIVCMD_MMAPBATCH_MFN_ERROR;
> >> +                  return __put_user(mfn, st->user_mfn++);
> >> +          } else
> >>                    st->user_mfn++;
> > 
> > Instead of having to resort to __get_user, couldn't you examine err and
> > base you the choice between PRIVCMD_MMAPBATCH_MFN_ERROR and
> > PRIVCMD_MMAPBATCH_PAGED_ERROR on it, similarly to what we did before?
> 
> The get_user() is required because we're or'ing in bits into the
> original user buffer.
> 
> However, I can see where your confusion comes from because it should be:
> 
>    mfn |= (err == -ENOENT) ? ...

Ah! I see now!


> > Also I think you should spend a few more words in the commit message
> > regarding the changes you are making to the error reporting path.
> 
> It's only moving around a bit.  I'm not sure what you want to be
> specifically called out here.

If it was just code movement, I wouldn't have to ask :-)
The additional get_user call for example would be something to write
down.


> >>  #ifdef CONFIG_XEN_AUTO_XLATE
> >> -int xen_xlate_remap_gfn_range(struct vm_area_struct *vma,
> >> +int xen_xlate_remap_gfn_array(struct vm_area_struct *vma,
> >>                          unsigned long addr,
> >> -                        xen_pfn_t gfn, int nr,
> >> -                        pgprot_t prot,
> >> -                        unsigned domid, 
> >> +                        xen_pfn_t *gfn, int nr,
> >> +                        int *err_ptr, pgprot_t prot,
> >> +                        unsigned domid,
> >>                          struct page **pages);
> >>  int xen_xlate_unmap_gfn_range(struct vm_area_struct *vma,
> >>                          int nr, struct page **pages);
> > 
> > Since we are introducing a new internal interface, shouldn't we try to
> > aim for something more generic, maybe like a scatter gather list?
> > We had one consecutive range and now we also have a list of pages. It
> > makes sense to jump straight into a list of ranges, right?
> 
> Again, I don't see why I should have to spend time of an even more
> generic interface we don't need.

Switching from a single range to a list of pages without considering to
jump directly to a list of ranges seems a bit short sighted to me.
Changing internal interfaces can be painful and since we are going
through that pain now, we might as well do it right.

That said, I am going to leave this with the other maintainers.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel