Re: [PATCH v6 01/11] memory: batch processing in acquire_resource()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2020 11:36, Roger Pau Monné wrote:
> On Tue, Jul 07, 2020 at 09:39:40PM +0200, Michał Leszczyński wrote:
>> @@ -1599,8 +1629,17 @@ long do_memory_op(unsigned long cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>  #endif
>>  
>>      case XENMEM_acquire_resource:
>> -        rc = acquire_resource(
>> -            guest_handle_cast(arg, xen_mem_acquire_resource_t));
>> +        do {
>> +            rc = acquire_resource(
>> +                guest_handle_cast(arg, xen_mem_acquire_resource_t),
>> +                &start_extent);
> 
> I think it would be interesting from a performance PoV to move the
> xmar copy_from_guest here, so that each call to acquire_resource
> in the loop doesn't need to perform a copy from guest. That's
> more relevant for translated callers, where a copy_from_guest involves
> a guest page table and a p2m walk.

This isn't just a nice-to-have for performance reasons, but a
correctness/consistency thing: A rogue (or buggy) guest may alter
the structure between two such reads. It _may_ be the case that
we're dealing fine with this right now, but it would feel like a
trap to fall into later on.

>> +
>> +            if ( hypercall_preempt_check() )
> 
> You are missing a rc == -ERESTART check here, you don't want to encode
> a continuation if rc is different than -ERESTART AFAICT.

At which point the subsequent containing do/while() likely wants
adjusting to, e.g. to "for( ; ; )".

Jan