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.

Fixes: 5427134eae ('x86: populate PVHv2 Dom0 physical memory map')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 549ff8ec7c..78d6f1012a 100644
--- 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();
     }
 
-- 
2.34.1

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.

> --- 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@suse.com>

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)?

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

Jan

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@suse.com>
> 
> 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.

On 07.02.2022 10:51, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 09:21:11AM +0100, Jan Beulich wrote:
>> On 05.02.2022 11:18, Roger Pau Monne wrote:
>>> --- 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@suse.com>
>>
>> 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.

Good point; please do so.

Jan