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

[David Vrabel <david.vrabel@citrix.com>]

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@citrix.com>
> 
> 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?

>> +			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) ? ...

> 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.

>>  #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.

David