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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jul 15, 2020 at 02:13:42PM +0200, Jan Beulich wrote:
> 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.

I *think* this is safe, given you copy from guest and perform the
checks for each iteration. I agree the copy should be pulled out of
the loop together with the checks. There's no point in performing it
for every iteration.

> >> +
> >> +            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( ; ; )".

That's another option, but you would need to add an extra
if ( rc != -ERESTART ) break; to the loop body if the while condition
is removed.

Roger.