[ovs-dev] [RFC PATCH] userspace: Define and use struct eth_addr.

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 28 22:02:54 UTC 2015


> On Aug 28, 2015, at 10:54 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> On Thu, Aug 27, 2015 at 06:29:16PM -0700, Jarno Rajahalme wrote:
>> Define struct eth_addr and use it instead of a uint8_t array for all
>> ethernet addresses in OVS userspace.  The struct is always the right
>> size, and it can be assigned without an explicit memcpy, which makes
>> code more readable.
>> 
>> "struct eth_addr" is a good type name for this as many utility
>> functions are already named accordingly.
>> 
>> struct eth_addr can be accessed as bytes as well as ovs_be16's, which
>> makes the struct 16-bit aligned.  All use seems to be 16-bit aligned,
>> so some algorithms on the ethernet addresses can be made a bit more
>> efficient making use of this fact.
>> 
>> As the struct fits into a register (in 64-bit systems) we pass it by
>> value when possible.
>> 
>> This patch also changes the few uses of Linux specific ETH_ALEN to
>> OVS's own ETH_ADDR_LEN, and removes the OFP_ETH_ALEN, as it is no
>> longer needed.
>> 
>> This work stemmed from a desire to make all struct flow members
>> assignable for unrelated exploration purposes.  However, I think this
>> might be a nice code readability improvement by itself.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> I like this.  I've thought about doing the same thing before and never
> got around to it.
> 
> I checked your claim about passing by value by looking at the x86-64
> ABI.  It's true!  Older ABIs were not so flexible--for example, I seem
> to recall that Borland C++ __fastcall ABI for Win32 would pass 1 and 2
> and 4 byte structs in a register, but not 3-byte ones.
> 
> However, GCC is almost criminally bad at optimizing it:
> 
>    blp at sigabrt:~/nicira/ovs/_build(0)$ cat tmp.c
>    struct x {
>        union {
>            unsigned char b[6];
>            unsigned short w[3];
>        };
>    };
>    void g(struct x);
>    void f(void)
>    {
>        struct x y = { { { 1, 2, 3, 4, 5, 6 } } };
>        g(y);
>    }
> 
>    blp at sigabrt:~/nicira/ovs/_build(0)$ gcc -O2 -g -m64 -c tmp.c
>    blp at sigabrt:~/nicira/ovs/_build(0)$ objdump -dr tmp.o
> 
>    tmp.o:     file format elf64-x86-64
> 
> 
>    Disassembly of section .text:
> 
>    0000000000000000 <f>:
>       0:	48 83 ec 18          	sub    $0x18,%rsp
>       4:	c6 04 24 01          	movb   $0x1,(%rsp)
>       8:	c6 44 24 01 02       	movb   $0x2,0x1(%rsp)
>       d:	c6 44 24 02 03       	movb   $0x3,0x2(%rsp)
>      12:	c6 44 24 03 04       	movb   $0x4,0x3(%rsp)
>      17:	c6 44 24 04 05       	movb   $0x5,0x4(%rsp)
>      1c:	c6 44 24 05 06       	movb   $0x6,0x5(%rsp)
>      21:	48 8b 3c 24          	mov    (%rsp),%rdi
>      25:	e8 00 00 00 00       	callq  2a <f+0x2a>
>                            26: R_X86_64_PC32	g-0x4
>      2a:	48 83 c4 18          	add    $0x18,%rsp
>      2e:	c3                   	retq   
> 
> Clang does better:
> 
>    blp at sigabrt:~/nicira/ovs/_build(0)$ clang -O2 -g -m64 -c tmp.c
>    blp at sigabrt:~/nicira/ovs/_build(0)$ objdump -dr tmp.o
> 
>    tmp.o:     file format elf64-x86-64
> 
> 
>    Disassembly of section .text:
> 
>    0000000000000000 <f>:
>       0:	48 bf 01 02 03 04 05 	movabs $0x60504030201,%rdi
>       7:	06 00 00 
>       a:	e9 00 00 00 00       	jmpq   f <f+0xf>
>                            b: R_X86_64_PC32	g-0x4
> 

One would hope that GCC would do a better job when inlining, though?

> Acked-by: Ben Pfaff <blp at nicira.com <mailto:blp at nicira.com>>

Thanks for the review. Unfortunately I forgot to add your Acked-by to the commit message, and I realized that just after pushing to github - now I don’t have anyone to shift blame to if builds start failing…

  Jarno




More information about the dev mailing list