As explained in the comments, a progress label wants to be before the function
it refers to for the higher level logic to make sense.  As it happens, the
effects are benign because gnttab_mappings is immediately adjacent to teardown
in terms of co-routine exit points.

There is and will always be a corner case with 0.  Help alleviate this
visually (at least slightly) with a BUILD_BUG_ON() to ensure the property
which makes this function do anything useful.

There is also a visual corner case when changing from PROGRESS() to
PROGRESS_VCPU().  The important detail is to check that there is a "return
rc;" logically between each PROGRESS*() marker.

Fixes: b1ee10be5625 ("gnttab: add preemption check to gnttab_release_mappings()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/common/domain.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 14b1341e53c6..0d3385ad5a66 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -419,11 +419,13 @@ static int domain_teardown(struct domain *d)
         };
 
     case PROG_none:
+        BUILD_BUG_ON(PROG_none != 0);
+
+    PROGRESS(gnttab_mappings):
         rc = gnttab_release_mappings(d);
         if ( rc )
             return rc;
 
-    PROGRESS(gnttab_mappings):
         for_each_vcpu ( d, v )
         {
             PROGRESS_VCPU(teardown);
-- 
2.11.0

On 27.08.2021 16:01, Andrew Cooper wrote:
> As explained in the comments, a progress label wants to be before the function
> it refers to for the higher level logic to make sense.  As it happens, the
> effects are benign because gnttab_mappings is immediately adjacent to teardown
> in terms of co-routine exit points.
> 
> There is and will always be a corner case with 0.  Help alleviate this
> visually (at least slightly) with a BUILD_BUG_ON() to ensure the property
> which makes this function do anything useful.
> 
> There is also a visual corner case when changing from PROGRESS() to
> PROGRESS_VCPU().  The important detail is to check that there is a "return
> rc;" logically between each PROGRESS*() marker.
> 
> Fixes: b1ee10be5625 ("gnttab: add preemption check to gnttab_release_mappings()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Despite the Fixes: tag I don't really view this as requiring backport.
Then again it would need to go to 4.15 only. Will need to make up my
mind ...

Jan

On 27/08/2021 15:07, Jan Beulich wrote:
> On 27.08.2021 16:01, Andrew Cooper wrote:
>> As explained in the comments, a progress label wants to be before the function
>> it refers to for the higher level logic to make sense.  As it happens, the
>> effects are benign because gnttab_mappings is immediately adjacent to teardown
>> in terms of co-routine exit points.
>>
>> There is and will always be a corner case with 0.  Help alleviate this
>> visually (at least slightly) with a BUILD_BUG_ON() to ensure the property
>> which makes this function do anything useful.
>>
>> There is also a visual corner case when changing from PROGRESS() to
>> PROGRESS_VCPU().  The important detail is to check that there is a "return
>> rc;" logically between each PROGRESS*() marker.
>>
>> Fixes: b1ee10be5625 ("gnttab: add preemption check to gnttab_release_mappings()")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> Despite the Fixes: tag I don't really view this as requiring backport.
> Then again it would need to go to 4.15 only. Will need to make up my
> mind ...

I'd prefer you to backport it.  Nothing good will come of having this
difference between 4.15 and 4.16.

Amongst other things, I'm not willing to bet we've found all the long
running loops during teardown.  After all, the gnttab one was staring me
in the face for years doing domain creation/destruction cleanup, before
I actually spotted it.

~Andrew