Re: [PATCH V3] xen/privcmd: improve performance of mapping of guest memory to dom0

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

On 22/11/12 08:42, Jan Beulich wrote:
>>>> On 21.11.12 at 18:19, Mats Petersson <mats.petersson@citrix.com> wrote:
>> @@ -2526,19 +2557,94 @@ 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) {
>> +				/* incrememnt done so we skip the error item */
> increment
>
>> +				done++;
>> +				if (err_ptr)
>> +					last_err = err_ptr[index] = err;
>> +				else
>> +					/* exit if error and no err_ptr */
>> +					goto out;
> For readability/reduction of indentation, I'd suggest
>
Yes, that's neater (spent far too long programming in environments where 
goto was frowned upon, so I end up taking different routes that don't 
make best use of it...)
>
>
> 				/* exit if error and no err_ptr */
> 				if (!err_ptr)
> 					goto out;
> 				/* increment done so we skip the error item */
> 				done++;
> 				last_err = err_ptr[index] = err;
>
> However, I wonder how a caller of the function would find out
> at which slot the error happened when not passing an error
> indicator array (after all, the function also doesn't undo what
> already succeeded in that case, i.e. the state is completely
> unknown to such a caller).
I agree, which is why the use of err_ptr is the preferred method - I 
have talked to Ian Campbell, and he agrees that the "old" interface, 
which doesn't use the err_ptr, should be removed eventually. However, we 
first have to remove the callers of that method, and since it's a 
different interface.

However, current callers of the "non-errptr" interface already has this 
problem regardless of these changes - there is no indication of how far 
it got in the existing code either, it just exits with an error code if 
one happens. Nothing has changed here except the code to fill in the 
err_ptr moved from privcmd.c to mmu.c. If the caller detects a fail, it 
will (or should, but I'm not aware of any code that doesn't at present) 
call munmap the region, which undoes the entire mapping.

We can't, at this point undo things, as we can't know how many previous 
blocks of mapping has been done to this VMA. The owner of the memory 
region may well have mappe several "bunches" of MFN's before the error, 
which this function only handles one "bunch" at the time. So it's up to 
the caller to deal with the error appropriately.

In summary, I don't see a reason to change it. And the caller still 
needs to call munmap to destroy the memory region it is mapping the MFNs 
into, so it wouldn't be much help to "undo" things here.

--
Mats

>
> Jan
>
>> +			}
>> +			batch_left -= done;
>> +			index += done;
>> +		} while (batch_left);
>