Re: [PATCH] Improve performance of IOCTL_PRIVCMD_MMAPBATCH_V2

[Mats Petersson <mats.petersson@citrix.com>]

On 21/11/12 10:29, Jan Beulich wrote:
>>>> On 16.11.12 at 15:45, Mats Petersson <mats.petersson@citrix.com> wrote:
>> @@ -2526,12 +2540,25 @@ int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>>   		if (err)
>>   			goto out;
>>   
>> -		err = HYPERVISOR_mmu_update(mmu_update, batch, NULL, domid);
>> -		if (err < 0)
>> -			goto out;
>> +		/* We record the error for each page that gives an error, but
>> +		 * continue mapping until the whole set is done */
>> +		do {
>> +			err = HYPERVISOR_mmu_update(&mmu_update[index],
>> +						    batch_left, &done, domid);
>> +			if (err < 0) {
>> +				if (err_ptr)
>> +					err_ptr[index] = err;
> Shouldn't you increment "done" here, in order to not retry the failed
> slot immediately?

Yes, good spot - for some reason, I have double checked the behaviour of 
"done", and it returns the index of the item which gave the error, not 
actually "how many were processed".

I have rewritten this part of code for V3 of this patch, but I think it 
still requires an increment of "done" to make it work correctly.
>
>> +				else /* exit if error and no err_ptr */
>> +					goto out;
>> +			}
>> +			batch_left -= done;
>> +			index += done;
>> +		} while (batch_left);
>>   
>>   		nr -= batch;
>>   		addr += range;
>> +		if (err_ptr)
>> +			err_ptr += batch;
>>   	}
>>   
>>   	err = 0;
>> @@ -303,6 +349,8 @@ static int mmap_return_errors_v1(void *data, void *state)
>>   	return __put_user(*mfnp, st->user_mfn++);
>>   }
>>   
>> +
>> +
> ???
This is cleaned up in the V3 patch.
>>   static struct vm_operations_struct privcmd_vm_ops;
>>   
>>   static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>> @@ -319,6 +367,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata,
>> int version)
>>   	if (!xen_initial_domain())
>>   		return -EPERM;
>>   
>> +
>> +
> ???
As above.

--
Mats
> Jan
>
>>   	switch (version) {
>>   	case 1:
>>   		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
>
>
>