Re: [PATCH] dom0/pvh: fix processing softirqs during memory map population

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

On Mon, Feb 07, 2022 at 09:21:11AM +0100, Jan Beulich wrote:
> On 05.02.2022 11:18, Roger Pau Monne wrote:
> > Make sure softirqs are processed at least once for every call to
> > pvh_populate_memory_range. It's likely that none of the calls to
> > pvh_populate_memory_range will perform 64 iterations, in which case
> > softirqs won't be processed for the whole duration of the p2m
> > population.
> > 
> > In order to force softirqs to be processed at least once for every
> > pvh_populate_memory_range call move the increasing of 'i' to be done
> > after evaluation, so on the first loop iteration softirqs will
> > unconditionally be processed.
> 
> Nit: The change still guarantees one invocation only for every
> iteration not encountering an error. That's fine from a functional
> pov, but doesn't fully match what you claim.

OK, will fix on next iteration.

> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -186,7 +186,7 @@ static int __init pvh_populate_memory_range(struct 
> > domain *d,
> >          start += 1UL << order;
> >          nr_pages -= 1UL << order;
> >          order_stats[order]++;
> > -        if ( (++i % MAP_MAX_ITER) == 0 )
> > +        if ( (i++ % MAP_MAX_ITER) == 0 )
> >              process_pending_softirqs();
> >      }
> 
> This way is perhaps easiest, so
> 
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> but I'd like you to consider to avoid doing this on the first
> iteration. How about keeping the code here as is, but instead
> insert an invocation in the sole caller (and there unconditionally
> at the end of every successful loop iteration)?

In fact I was thinking that we should call process_pending_softirqs on
every iteration: the calls to guest_physmap_add_page could use a 1G
page order, so if not using sync-pt (at least until your series for
IOMMU super-page support is committed) mapping a whole 1G page using
4K chunks on the IOMMU page-tables could be quite time consuming, and
hence we would likely need to process softirqs on every iteration.

> 
> Furthermore, how about taking the opportunity and deleting the mis-
> named and single-use-only MAP_MAX_ITER define?

Right, let me know your opinion about the comment above.

Thanks, Roger.