[v4,6/9] x86/mm: add an end_of_loop label in map_pages_to_xen

Commit Message

Xia, Hongyan Dec. 4, 2019, 5:10 p.m. UTC

From: Wei Liu <[e-mail]>

We will soon need to clean up mappings whenever the out most loop is
ended. Add a new label and turn relevant continue's into goto's.

No functional change.

Signed-off-by: Wei Liu <[e-mail]>
---
 xen/arch/x86/mm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Xia, Hongyan Dec. 5, 2019, 10:21 a.m. UTC | #1

>On 02.10.2019 19:16, Hongyan Xia wrote:
>> We will soon need to clean up mappings whenever the out most loop is
>> ended. Add a new label and turn relevant continue's into goto's.
>
>I think already when this still was RFC I did indicate that I'm not
>happy about the introduction of these labels (including also patch 8).
>I realize it's quite a lot to ask, but both functions would benefit
>from splitting up into per-level helper functions, which - afaict -
>would avoid the need for such labels, and which would at the same
>time likely make it quite a bit easier to extend these to the
>5-level page tables case down the road.
>
>Thoughts?
>
>Jan

A common pattern I have found when mapping PTE pages on-demand (and I
think is the exact intention of these labels from Wei, also described
in the commit message) is that we often need to do:

map some pages - process those pages - error occurs or this iteration
of loop can be skipped - _clean up the mappings_ - continue or return

As long as cleaning up is required, these labels will likely be needed
as the clean-up path before skipping or returning, so I would say we
will see such labels even if we split it into helper functions
(virt_to_xen_l[123]e() later in the patch series is an example). I see
the labels more or less as orthogonal to modularising into helper
functions.

I would also like to see some helpers because these functions are
getting too long and convoluted, but I wonder if they could be another
mini-series outside the directmap-removal series.

Hongyan

Jan Beulich Dec. 5, 2019, 10:25 a.m. UTC | #2

On 05.12.2019 11:21, Xia, Hongyan wrote:
>> On 02.10.2019 19:16, Hongyan Xia wrote:
>>> We will soon need to clean up mappings whenever the out most loop is
>>> ended. Add a new label and turn relevant continue's into goto's.
>>
>> I think already when this still was RFC I did indicate that I'm not
>> happy about the introduction of these labels (including also patch 8).
>> I realize it's quite a lot to ask, but both functions would benefit
>>from splitting up into per-level helper functions, which - afaict -
>> would avoid the need for such labels, and which would at the same
>> time likely make it quite a bit easier to extend these to the
>> 5-level page tables case down the road.
> 
> A common pattern I have found when mapping PTE pages on-demand (and I
> think is the exact intention of these labels from Wei, also described
> in the commit message) is that we often need to do:
> 
> map some pages - process those pages - error occurs or this iteration
> of loop can be skipped - _clean up the mappings_ - continue or return
> 
> As long as cleaning up is required, these labels will likely be needed
> as the clean-up path before skipping or returning, so I would say we
> will see such labels even if we split it into helper functions
> (virt_to_xen_l[123]e() later in the patch series is an example). I see
> the labels more or less as orthogonal to modularising into helper
> functions.

I think differently: The fact that labels are needed is because of
the complexity of the functions. Simpler functions would allow
goto-free handling of such error conditions (by instead being able
to use continue, break, or return without making the code less
readable, often even improving readability).

Jan

Paul Durrant Dec. 5, 2019, 11:02 a.m. UTC | #3

> -----Original Message-----
> From: Xen-devel <[e-mail]> On Behalf Of Jan
> Beulich
> Sent: 05 December 2019 10:26
> To: Xia, Hongyan <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label
> in map_pages_to_xen
> 
> On 05.12.2019 11:21, Xia, Hongyan wrote:
> >> On 02.10.2019 19:16, Hongyan Xia wrote:
> >>> We will soon need to clean up mappings whenever the out most loop is
> >>> ended. Add a new label and turn relevant continue's into goto's.
> >>
> >> I think already when this still was RFC I did indicate that I'm not
> >> happy about the introduction of these labels (including also patch 8).
> >> I realize it's quite a lot to ask, but both functions would benefit
> >>from splitting up into per-level helper functions, which - afaict -
> >> would avoid the need for such labels, and which would at the same
> >> time likely make it quite a bit easier to extend these to the
> >> 5-level page tables case down the road.
> >
> > A common pattern I have found when mapping PTE pages on-demand (and I
> > think is the exact intention of these labels from Wei, also described
> > in the commit message) is that we often need to do:
> >
> > map some pages - process those pages - error occurs or this iteration
> > of loop can be skipped - _clean up the mappings_ - continue or return
> >
> > As long as cleaning up is required, these labels will likely be needed
> > as the clean-up path before skipping or returning, so I would say we
> > will see such labels even if we split it into helper functions
> > (virt_to_xen_l[123]e() later in the patch series is an example). I see
> > the labels more or less as orthogonal to modularising into helper
> > functions.
> 
> I think differently: The fact that labels are needed is because of
> the complexity of the functions. Simpler functions would allow
> goto-free handling of such error conditions (by instead being able
> to use continue, break, or return without making the code less
> readable, often even improving readability).

And what is wrong with using goto-s? It is a *very* common style of error handling use widely in e.g. the linux kernel. IMO it often makes error paths much more obvious and easier to reason about. In fact I very much dislike returns from the middle of functions as they can easily lead to avoidance of necessary error cleanup.

  Paul

> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Jan Beulich Dec. 5, 2019, 11:12 a.m. UTC | #4

On 05.12.2019 12:02, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <[e-mail]> On Behalf Of Jan
>> Beulich
>> Sent: 05 December 2019 10:26
>> To: Xia, Hongyan <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an end_of_loop label
>> in map_pages_to_xen
>>
>> On 05.12.2019 11:21, Xia, Hongyan wrote:
>>>> On 02.10.2019 19:16, Hongyan Xia wrote:
>>>>> We will soon need to clean up mappings whenever the out most loop is
>>>>> ended. Add a new label and turn relevant continue's into goto's.
>>>>
>>>> I think already when this still was RFC I did indicate that I'm not
>>>> happy about the introduction of these labels (including also patch 8).
>>>> I realize it's quite a lot to ask, but both functions would benefit
>>> >from splitting up into per-level helper functions, which - afaict -
>>>> would avoid the need for such labels, and which would at the same
>>>> time likely make it quite a bit easier to extend these to the
>>>> 5-level page tables case down the road.
>>>
>>> A common pattern I have found when mapping PTE pages on-demand (and I
>>> think is the exact intention of these labels from Wei, also described
>>> in the commit message) is that we often need to do:
>>>
>>> map some pages - process those pages - error occurs or this iteration
>>> of loop can be skipped - _clean up the mappings_ - continue or return
>>>
>>> As long as cleaning up is required, these labels will likely be needed
>>> as the clean-up path before skipping or returning, so I would say we
>>> will see such labels even if we split it into helper functions
>>> (virt_to_xen_l[123]e() later in the patch series is an example). I see
>>> the labels more or less as orthogonal to modularising into helper
>>> functions.
>>
>> I think differently: The fact that labels are needed is because of
>> the complexity of the functions. Simpler functions would allow
>> goto-free handling of such error conditions (by instead being able
>> to use continue, break, or return without making the code less
>> readable, often even improving readability).
> 
> And what is wrong with using goto-s? It is a *very* common style of
> error handling use widely in e.g. the linux kernel. IMO it often
> makes error paths much more obvious and easier to reason about. In
> fact I very much dislike returns from the middle of functions as
> they can easily lead to avoidance of necessary error cleanup.

Whereas I personally dislike goto-s (and I've been taught so when
first learning programming languages). In private code I avoid them
by all means. In projects I'm the maintainer for I accept them when
the alternative is noticeably more ugly.

Jan

Xia, Hongyan Dec. 5, 2019, 1:22 p.m. UTC | #5

I mean... I was taught so as well but I was also taught an exception
which is using it for error handling and cleaning up. I am not sure if
using alternatives would result in cleaner code in this situation.

Hongyan

On Thu, 2019-12-05 at 12:12 +0100, Jan Beulich wrote:
> On 05.12.2019 12:02, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <[e-mail]> On
> > > Behalf Of Jan
> > > Beulich
> > > Sent: 05 December 2019 10:26
> > > To: Xia, Hongyan <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; 
> > > [e-mail];
> > > [e-mail]
> > > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an
> > > end_of_loop label
> > > in map_pages_to_xen
> > > 
> > > On 05.12.2019 11:21, Xia, Hongyan wrote:
> > > > > On 02.10.2019 19:16, Hongyan Xia wrote:
> > > > > > We will soon need to clean up mappings whenever the out
> > > > > > most loop is
> > > > > > ended. Add a new label and turn relevant continue's into
> > > > > > goto's.
> > > > > 
> > > > > I think already when this still was RFC I did indicate that
> > > > > I'm not
> > > > > happy about the introduction of these labels (including also
> > > > > patch 8).
> > > > > I realize it's quite a lot to ask, but both functions would
> > > > > benefit
> > > > > from splitting up into per-level helper functions, which -
> > > > > afaict -
> > > > > would avoid the need for such labels, and which would at the
> > > > > same
> > > > > time likely make it quite a bit easier to extend these to the
> > > > > 5-level page tables case down the road.
> > > > 
> > > > A common pattern I have found when mapping PTE pages on-demand
> > > > (and I
> > > > think is the exact intention of these labels from Wei, also
> > > > described
> > > > in the commit message) is that we often need to do:
> > > > 
> > > > map some pages - process those pages - error occurs or this
> > > > iteration
> > > > of loop can be skipped - _clean up the mappings_ - continue or
> > > > return
> > > > 
> > > > As long as cleaning up is required, these labels will likely be
> > > > needed
> > > > as the clean-up path before skipping or returning, so I would
> > > > say we
> > > > will see such labels even if we split it into helper functions
> > > > (virt_to_xen_l[123]e() later in the patch series is an
> > > > example). I see
> > > > the labels more or less as orthogonal to modularising into
> > > > helper
> > > > functions.
> > > 
> > > I think differently: The fact that labels are needed is because
> > > of
> > > the complexity of the functions. Simpler functions would allow
> > > goto-free handling of such error conditions (by instead being
> > > able
> > > to use continue, break, or return without making the code less
> > > readable, often even improving readability).
> > 
> > And what is wrong with using goto-s? It is a *very* common style of
> > error handling use widely in e.g. the linux kernel. IMO it often
> > makes error paths much more obvious and easier to reason about. In
> > fact I very much dislike returns from the middle of functions as
> > they can easily lead to avoidance of necessary error cleanup.
> 
> Whereas I personally dislike goto-s (and I've been taught so when
> first learning programming languages). In private code I avoid them
> by all means. In projects I'm the maintainer for I accept them when
> the alternative is noticeably more ugly.
> 
> Jan

Xia, Hongyan Dec. 6, 2019, 3:58 p.m. UTC | #6

Thanks for the suggestions. I will attempt to restructure the code.

Hongyan

On Thu, 2019-12-05 at 12:12 +0100, Jan Beulich wrote:
> On 05.12.2019 12:02, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Xen-devel <[e-mail]> On
> > > Behalf Of Jan
> > > Beulich
> > > Sent: 05 December 2019 10:26
> > > To: Xia, Hongyan <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; 
> > > [e-mail];
> > > [e-mail]
> > > Subject: Re: [Xen-devel] [PATCH v4 6/9] x86/mm: add an
> > > end_of_loop label
> > > in map_pages_to_xen
> > > 
> > > On 05.12.2019 11:21, Xia, Hongyan wrote:
> > > > > On 02.10.2019 19:16, Hongyan Xia wrote:
> > > > > > We will soon need to clean up mappings whenever the out
> > > > > > most loop is
> > > > > > ended. Add a new label and turn relevant continue's into
> > > > > > goto's.
> > > > > 
> > > > > I think already when this still was RFC I did indicate that
> > > > > I'm not
> > > > > happy about the introduction of these labels (including also
> > > > > patch 8).
> > > > > I realize it's quite a lot to ask, but both functions would
> > > > > benefit
> > > > > from splitting up into per-level helper functions, which -
> > > > > afaict -
> > > > > would avoid the need for such labels, and which would at the
> > > > > same
> > > > > time likely make it quite a bit easier to extend these to the
> > > > > 5-level page tables case down the road.
> > > > 
> > > > A common pattern I have found when mapping PTE pages on-demand
> > > > (and I
> > > > think is the exact intention of these labels from Wei, also
> > > > described
> > > > in the commit message) is that we often need to do:
> > > > 
> > > > map some pages - process those pages - error occurs or this
> > > > iteration
> > > > of loop can be skipped - _clean up the mappings_ - continue or
> > > > return
> > > > 
> > > > As long as cleaning up is required, these labels will likely be
> > > > needed
> > > > as the clean-up path before skipping or returning, so I would
> > > > say we
> > > > will see such labels even if we split it into helper functions
> > > > (virt_to_xen_l[123]e() later in the patch series is an
> > > > example). I see
> > > > the labels more or less as orthogonal to modularising into
> > > > helper
> > > > functions.
> > > 
> > > I think differently: The fact that labels are needed is because
> > > of
> > > the complexity of the functions. Simpler functions would allow
> > > goto-free handling of such error conditions (by instead being
> > > able
> > > to use continue, break, or return without making the code less
> > > readable, often even improving readability).
> > 
> > And what is wrong with using goto-s? It is a *very* common style of
> > error handling use widely in e.g. the linux kernel. IMO it often
> > makes error paths much more obvious and easier to reason about. In
> > fact I very much dislike returns from the middle of functions as
> > they can easily lead to avoidance of necessary error cleanup.
> 
> Whereas I personally dislike goto-s (and I've been taught so when
> first learning programming languages). In private code I avoid them
> by all means. In projects I'm the maintainer for I accept them when
> the alternative is noticeably more ugly.
> 
> Jan

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f7464c2103..f530dd391c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5273,7 +5273,7 @@  int map_pages_to_xen(
             if ( !mfn_eq(mfn, INVALID_MFN) )
                 mfn  = mfn_add(mfn, 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
             nr_mfns -= 1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT);
-            continue;
+            goto end_of_loop;
         }
 
         if ( (l3e_get_flags(ol3e) & _PAGE_PRESENT) &&
@@ -5300,7 +5300,7 @@  int map_pages_to_xen(
                 if ( !mfn_eq(mfn, INVALID_MFN) )
                     mfn = mfn_add(mfn, i);
                 nr_mfns -= i;
-                continue;
+                goto end_of_loop;
             }
 
             l2t = alloc_xen_pagetable();
@@ -5469,7 +5469,7 @@  int map_pages_to_xen(
                 {
                     if ( locking )
                         spin_unlock(&map_pgdir_lock);
-                    continue;
+                    goto end_of_loop;
                 }
 
                 if ( l2e_get_flags(ol2e) & _PAGE_PSE )
@@ -5524,7 +5524,7 @@  int map_pages_to_xen(
             {
                 if ( locking )
                     spin_unlock(&map_pgdir_lock);
-                continue;
+                goto end_of_loop;
             }
 
             l2t = l3e_to_l2e(ol3e);
@@ -5549,6 +5549,7 @@  int map_pages_to_xen(
             else if ( locking )
                 spin_unlock(&map_pgdir_lock);
         }
+    end_of_loop:;
     }
 
 #undef flush_flags