[v6,11/15] x86/smpboot: clone_mapping should have one exit path

Commit Message

Hongyan Xia April 24, 2020, 2:09 p.m. UTC

From: Wei Liu <[e-mail]>

We will soon need to clean up page table mappings in the exit path.

No functional change.

Signed-off-by: Wei Liu <[e-mail]>
Signed-off-by: Hongyan Xia <[e-mail]>
---
 xen/arch/x86/smpboot.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Jan Beulich April 30, 2020, 2:59 p.m. UTC | #1

On 24.04.2020 16:09, Hongyan Xia wrote:
> From: Wei Liu <[e-mail]>

Like for patches 1 and 2 in this series, and as iirc mentioned already
long ago for those, "should" or alike are misleading here: It's not a
mistake that they don't, i.e. this is not a bug fix. You _want_ these
functions to have a single (main) exit path, for subsequent changes of
yours ending up easier.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -675,6 +675,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>      l3_pgentry_t *pl3e;
>      l2_pgentry_t *pl2e;
>      l1_pgentry_t *pl1e;
> +    int rc = -ENOMEM;

Would you mind starting out with 0 here, ...

> @@ -715,7 +716,10 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>              pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
>              flags = l1e_get_flags(*pl1e);
>              if ( !(flags & _PAGE_PRESENT) )
> -                return 0;
> +            {
> +                rc = 0;
> +                goto out;
> +            }

... dropping assignment and braces here, and ...

> @@ -724,7 +728,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>      {
>          pl3e = alloc_xen_pagetable();
>          if ( !pl3e )
> -            return -ENOMEM;
> +            goto out;

... setting rc to -ENOMEM ahead of the if() up from here?
This imo makes things then not only minimally shorter, but
also fights slightly better with the nevertheless still
existing (after this patch) separate early exit paths.

Jan

Patch

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 275ce7661d..5b0e24f925 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -675,6 +675,7 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
     l3_pgentry_t *pl3e;
     l2_pgentry_t *pl2e;
     l1_pgentry_t *pl1e;
+    int rc = -ENOMEM;
 
     /*
      * Sanity check 'linear'.  We only allow cloning from the Xen virtual
@@ -715,7 +716,10 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
             pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
             flags = l1e_get_flags(*pl1e);
             if ( !(flags & _PAGE_PRESENT) )
-                return 0;
+            {
+                rc = 0;
+                goto out;
+            }
             pfn = l1e_get_pfn(*pl1e);
         }
     }
@@ -724,7 +728,7 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
     {
         pl3e = alloc_xen_pagetable();
         if ( !pl3e )
-            return -ENOMEM;
+            goto out;
         clear_page(pl3e);
         l4e_write(&rpt[root_table_offset(linear)],
                   l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
@@ -738,7 +742,7 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
     {
         pl2e = alloc_xen_pagetable();
         if ( !pl2e )
-            return -ENOMEM;
+            goto out;
         clear_page(pl2e);
         l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
     }
@@ -754,7 +758,7 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
     {
         pl1e = alloc_xen_pagetable();
         if ( !pl1e )
-            return -ENOMEM;
+            goto out;
         clear_page(pl1e);
         l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
     }
@@ -775,7 +779,9 @@  static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
     else
         l1e_write(pl1e, l1e_from_pfn(pfn, flags));
 
-    return 0;
+    rc = 0;
+ out:
+    return rc;
 }
 
 DEFINE_PER_CPU(root_pgentry_t *, root_pgt);