[PATCH] memory: avoid pointless continuation in xenmem_add_to_physmap()

[PATCH] memory: avoid pointless continuation in xenmem_add_to_physmap()

[Jan Beulich]

Adjust so we uniformly avoid needlessly arranging for a continuation on
the last iteration.

Fixes: 5777a3742d88 ("IOMMU: hold page ref until after deferred TLB flush")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -854,8 +854,9 @@ int xenmem_add_to_physmap(struct domain
             ++extra.ppage;
 
         /* Check for continuation if it's not the last iteration. */
-        if ( (++done >= ARRAY_SIZE(pages) && extra.ppage) ||
-             (xatp->size > done && hypercall_preempt_check()) )
+        if ( xatp->size > ++done &&
+             ((done >= ARRAY_SIZE(pages) && extra.ppage) ||
+              hypercall_preempt_check()) )
         {
             rc = start + done;
             break;

Re: [PATCH] memory: avoid pointless continuation in xenmem_add_to_physmap()

[Julien Grall]

Hi Jan,

On 04/12/2020 10:43, Jan Beulich wrote:
> Adjust so we uniformly avoid needlessly arranging for a continuation on
> the last iteration.
> 
> Fixes: 5777a3742d88 ("IOMMU: hold page ref until after deferred TLB flush")

I view this patch as an optimization because there is nothing wrong with 
the code. So I think "Fixes" is not entirely suitable here.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Other than my remark about the tag:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

> 
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -854,8 +854,9 @@ int xenmem_add_to_physmap(struct domain
>               ++extra.ppage;
>   
>           /* Check for continuation if it's not the last iteration. */
> -        if ( (++done >= ARRAY_SIZE(pages) && extra.ppage) ||
> -             (xatp->size > done && hypercall_preempt_check()) )
> +        if ( xatp->size > ++done &&
> +             ((done >= ARRAY_SIZE(pages) && extra.ppage) ||
> +              hypercall_preempt_check()) )
>           {
>               rc = start + done;
>               break;
> 

-- 
Julien Grall

Re: [PATCH] memory: avoid pointless continuation in xenmem_add_to_physmap()

[Jan Beulich]

On 13.01.2021 20:09, Julien Grall wrote:
> On 04/12/2020 10:43, Jan Beulich wrote:
>> Adjust so we uniformly avoid needlessly arranging for a continuation on
>> the last iteration.
>>
>> Fixes: 5777a3742d88 ("IOMMU: hold page ref until after deferred TLB flush")
> 
> I view this patch as an optimization because there is nothing wrong with 
> the code. So I think "Fixes" is not entirely suitable here.

Well, yes, I think one can look at it both ways. The code was
clearly worse than necessary, performance wise. We should have
recognized this (as well as what had caused XSA-355) before
XSA-346 was even predisclosed, and the code should never have
been the way it is right now. So while it doesn't result in any
malfunction (other than the XSA-355 issue), I still view it as
a bug. But I can see that one may also view it differently.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Other than my remark about the tag:
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks.

Jan