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

Ben Pfaff blp at nicira.com
Fri Aug 28 17:54:19 UTC 2015


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

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



More information about the dev mailing list