> On Aug 28, 2015, at 10:54 AM, Ben Pfaff <b...@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 <jrajaha...@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@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@sigabrt:~/nicira/ovs/_build(0)$ gcc -O2 -g -m64 -c tmp.c
> blp@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@sigabrt:~/nicira/ovs/_build(0)$ clang -O2 -g -m64 -c tmp.c
> blp@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 <b...@nicira.com <mailto:b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
