Re: [PATCH 02/20] PVH xen: add XENMEM_add_to_physmap_range

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 17 May 2013 07:37:16 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 17.05.13 at 01:56, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Thu, 16 May 2013 08:21:16 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 16.05.13 at 01:05, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > On Wed, 15 May 2013 10:58:43 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> > 
> >> >xatpr->size-1, 1) )
> >> 
> >> ... you can read directly into the respective xatp fields here.
> > 
> > I could, but it makes the lines long/wrap in the if statement
> > making the code
> > harder to read IMO. The compiler should do exact same thing in both
> > cases.
> 
> I'm afraid it's not permitted to do so because of the addresses of
> the variables being taken and, through some macro levels, passed
> to global functions.
> 
> > If it really bothers you, I can change it.
> 
> It's certainly a matter of taste to some degree, but to me it's
> inefficient code (unless you could prove at least modern gcc indeed
> doing said optimization despite the use of the & operator) and
> would sooner or later (once stumbling across that code again)
> prompt me to submit a cleanup patch...

Modern compilers are amazing, it generates pretty much the same code, 
slightly different ordering, and only two instructions are different:

NEW (meaning not using local variables):

        if ( copy_from_guest_offset(&xatp.idx, xatpr->idxs, xatpr->size-1, 1)
             || copy_from_guest_offset(&xatp.gpfn, xatpr->gpfns, xatpr->size-1,
                                       1) )
            return -EFAULT;


0xffff82c4c017198b <xenmem_add_to_physmap_range+31>:    mov    %rax,%r12
0xffff82c4c017198e <xenmem_add_to_physmap_range+34>:    and    %rsp,%r12
0xffff82c4c0171991 <xenmem_add_to_physmap_range+37>:    lea    -0x50(%rbp),%r15
0xffff82c4c0171995 <xenmem_add_to_physmap_range+41>:    lea    0x8(%r15),%rdx
0xffff82c4c0171999 <xenmem_add_to_physmap_range+45>:    mov    %rdx,-0x58(%rbp)
0xffff82c4c017199d <xenmem_add_to_physmap_range+49>:    mov    %r12,%r13

0xffff82c4c01719a0 <xenmem_add_to_physmap_range+52>:    lea    0x10(%r15),%rdx
0xffff82c4c01719a0 <xenmem_add_to_physmap_range+52>:    lea    -0x60(%rbp),%rdx
                      ^^^^^^^^^ Above is OLD with local variables.

0xffff82c4c01719a4 <xenmem_add_to_physmap_range+56>:    mov    %rdx,-0x60(%rbp)
0xffff82c4c01719a8 <xenmem_add_to_physmap_range+60>:    mov    %r12,%r14
0xffff82c4c01719ab <xenmem_add_to_physmap_range+63>:    lea    -0x34(%rbp),%rdx
0xffff82c4c01719af <xenmem_add_to_physmap_range+67>:    mov    %rdx,-0x70(%rbp)
0xffff82c4c01719b3 <xenmem_add_to_physmap_range+71>:    mov    %r12,-0x78(%rbp)
0xffff82c4c01719b7 <xenmem_add_to_physmap_range+75>:    jmpq   0xffff82c4c0171b6a <xenmem_add_to_physmap_range+510 at mm.c:4685>
0xffff82c4c01719bc <xenmem_add_to_physmap_range+80>:    mov    0x8(%rbx),%rdx
0xffff82c4c01719c0 <xenmem_add_to_physmap_range+84>:    mov    0x7fe8(%r12),%rcx
0xffff82c4c01719c8 <xenmem_add_to_physmap_range+92>:    mov    0x10(%rcx),%rcx
0xffff82c4c01719cc <xenmem_add_to_physmap_range+96>:    cmpb   $0x0,0x1e8(%rcx)
0xffff82c4c01719d3 <xenmem_add_to_physmap_range+103>:   je     0xffff82c4c01719ed <xenmem_add_to_physmap_range+129 at mm.c:4689>
0xffff82c4c01719d5 <xenmem_add_to_physmap_range+105>:   movzwl %ax,%eax
0xffff82c4c01719d8 <xenmem_add_to_physmap_range+108>:   lea    -0x8(%rdx,%rax,8),%rsi
0xffff82c4c01719dd <xenmem_add_to_physmap_range+113>:   mov    $0x8,%edx

0xffff82c4c01719e2 <xenmem_add_to_physmap_range+118>:   mov    -0x58(%rbp),%rdi
0xffff82c4c01719fc <xenmem_add_to_physmap_range+144>:   mov    %r15,%rdi
                      ^^^^^^^^^ Above is OLD with local variables.

0xffff82c4c01719e6 <xenmem_add_to_physmap_range+122>:   callq  0xffff82c4c01b5861 <copy_from_user_hvm at hvm.c:2739>
0xffff82c4c01719eb <xenmem_add_to_physmap_range+127>:   jmp    0xffff82c4c0171a03 <xenmem_add_to_physmap_range+151 at mm.c:4689>
0xffff82c4c01719ed <xenmem_add_to_physmap_range+129>:   movzwl %ax,%eax
0xffff82c4c01719f0 <xenmem_add_to_physmap_range+132>:   lea    -0x8(%rdx,%rax,8),%rsi
0xffff82c4c01719f5 <xenmem_add_to_physmap_range+137>:   mov    $0x8,%edx
0xffff82c4c01719fa <xenmem_add_to_physmap_range+142>:   mov    -0x58(%rbp),%rdi
0xffff82c4c01719fe <xenmem_add_to_physmap_range+146>:   callq  0xffff82c4c018c59b <copy_from_user at usercopy.c:167>

Since, I already changed it, I'll leave it changed, but now I'm going crazy
whether the '||' belongs on line by itself, and the CODING_STYE doesn't
say anything, so hopefully I've it right and not have to crank out
another version just for that :).

thanks,
Mukesh