From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
To: Joe Perches <joe@perches.com>
Cc: grygorii.strashko@ti.com, davem@davemloft.net,
linux-omap@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions
Date: Fri, 27 Jul 2018 23:23:49 +0300 [thread overview]
Message-ID: <20180727202348.GC2619@khorivan> (raw)
In-Reply-To: <f22d3024773abbd811673c4eb957e4a5ffca1387.camel@perches.com>
On Fri, Jul 27, 2018 at 01:04:22PM -0700, Joe Perches wrote:
>On Fri, 2018-07-27 at 22:36 +0300, Ivan Khoronzhuk wrote:
>> On Fri, Jul 27, 2018 at 12:21:07PM -0700, Joe Perches wrote:
>> > On Fri, 2018-07-27 at 22:13 +0300, Ivan Khoronzhuk wrote:
>> > > Replace ugly macroses on functions.
>> >
>> > Careful, see below.
>> >
>> > > diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>> >
>> > []
>> > > @@ -565,40 +565,40 @@ static const struct cpsw_stats cpsw_gstrings_ch_stats[] = {
>> > > (func)(slave++, ##arg); \
>> > > } while (0)
>> > >
>> > > -#define cpsw_dual_emac_src_port_detect(cpsw, status, ndev, skb) \
>> > > - do { \
>> > > - if (!cpsw->data.dual_emac) \
>> > > - break; \
>> > > - if (CPDMA_RX_SOURCE_PORT(status) == 1) { \
>> > > - ndev = cpsw->slaves[0].ndev; \
>> > > - skb->dev = ndev; \
>> > > - } else if (CPDMA_RX_SOURCE_PORT(status) == 2) { \
>> > > - ndev = cpsw->slaves[1].ndev; \
>> > > - skb->dev = ndev; \
>> > > - } \
>> > > - } while (0)
>> > > -#define cpsw_add_mcast(cpsw, priv, addr) \
>> > > - do { \
>> > > - if (cpsw->data.dual_emac) { \
>> > > - struct cpsw_slave *slave = cpsw->slaves + \
>> > > - priv->emac_port; \
>> > > - int slave_port = cpsw_get_slave_port( \
>> > > - slave->slave_num); \
>> > > - cpsw_ale_add_mcast(cpsw->ale, addr, \
>> > > - 1 << slave_port | ALE_PORT_HOST, \
>> > > - ALE_VLAN, slave->port_vlan, 0); \
>> > > - } else { \
>> > > - cpsw_ale_add_mcast(cpsw->ale, addr, \
>> > > - ALE_ALL_PORTS, \
>> > > - 0, 0, 0); \
>> > > - } \
>> > > - } while (0)
>> > > -
>> > > static inline int cpsw_get_slave_port(u32 slave_num)
>> > > {
>> > > return slave_num + 1;
>> > > }
>> > >
>> > > +static inline void cpsw_src_port_detect(struct cpsw_common *cpsw, int status,
>> > > + struct sk_buff *skb)
>> > > +{
>> > > + if (!cpsw->data.dual_emac)
>> > > + return;
>> > > +
>> > > + if (CPDMA_RX_SOURCE_PORT(status) == 1)
>> > > + skb->dev = cpsw->slaves[0].ndev;
>> > > + else if (CPDMA_RX_SOURCE_PORT(status) == 2)
>> > > + skb->dev = cpsw->slaves[1].ndev;
>> > > +}
>> >
>> > perhaps better as a switch/case
>>
>> not better, it's shorter.
>
>True for the source code but it compiles to more object code.
>
>$ cat foo.c
>struct cpsw_common {
> struct {
> int dual_emac;
> } data;
> struct {
> int ndev;
> } slaves[2];
>};
>
>struct sk_buff {
> int dev;
>};
>
>#define CPDMA_RX_SOURCE_PORT(__status__) ((__status__ >> 16) & 0x7)
>
>#if defined SWITCH
>
>void foo(struct cpsw_common *cpsw, int status, struct sk_buff *skb)
>{
> if (!cpsw->data.dual_emac)
> return;
>
> switch (CPDMA_RX_SOURCE_PORT(status)) {
> case 1:
> skb->dev = cpsw->slaves[0].ndev;
> break;
> case 2:
> skb->dev = cpsw->slaves[1].ndev;
> break;
> }
>}
>
>#else
>
>void foo(struct cpsw_common *cpsw, int status, struct sk_buff *skb)
>{
> if (!cpsw->data.dual_emac)
> return;
>
> if (CPDMA_RX_SOURCE_PORT(status) == 1)
> skb->dev = cpsw->slaves[0].ndev;
> else if (CPDMA_RX_SOURCE_PORT(status) == 2)
> skb->dev = cpsw->slaves[1].ndev;
>}
>
>#endif
>$ gcc -c -O2 -DSWITCH foo.c
>$ size foo.o
> text data bss dec hex filename
> 94 0 0 94 5e foo.o
>$ objdump -d foo.o
>
>foo.o: file format elf64-x86-64
>
>
>Disassembly of section .text:
>
>0000000000000000 <foo>:
> 0: 8b 07 mov (%rdi),%eax
> 2: 85 c0 test %eax,%eax
> 4: 74 15 je 1b <foo+0x1b>
> 6: c1 fe 10 sar $0x10,%esi
> 9: 83 e6 07 and $0x7,%esi
> c: 83 fe 01 cmp $0x1,%esi
> f: 74 17 je 28 <foo+0x28>
> 11: 83 fe 02 cmp $0x2,%esi
> 14: 75 0a jne 20 <foo+0x20>
> 16: 8b 47 08 mov 0x8(%rdi),%eax
> 19: 89 02 mov %eax,(%rdx)
> 1b: f3 c3 repz retq
> 1d: 0f 1f 00 nopl (%rax)
> 20: f3 c3 repz retq
> 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
> 28: 8b 47 04 mov 0x4(%rdi),%eax
> 2b: 89 02 mov %eax,(%rdx)
> 2d: c3 retq
>$ gcc -c -O2 foo.c
>$ size foo.o
> text data bss dec hex filename
> 102 0 0 102 66 foo.o
>$ objdump -d foo.o
>
>foo.o: file format elf64-x86-64
>
>
>Disassembly of section .text:
>
>0000000000000000 <foo>:
> 0: 8b 07 mov (%rdi),%eax
> 2: 85 c0 test %eax,%eax
> 4: 74 10 je 16 <foo+0x16>
> 6: c1 fe 10 sar $0x10,%esi
> 9: 83 e6 07 and $0x7,%esi
> c: 83 fe 01 cmp $0x1,%esi
> f: 74 0f je 20 <foo+0x20>
> 11: 83 fe 02 cmp $0x2,%esi
> 14: 74 1a je 30 <foo+0x30>
> 16: f3 c3 repz retq
> 18: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1)
> 1f: 00
> 20: 8b 47 04 mov 0x4(%rdi),%eax
> 23: 89 02 mov %eax,(%rdx)
> 25: c3 retq
> 26: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
> 2d: 00 00 00
> 30: 8b 47 08 mov 0x8(%rdi),%eax
> 33: 89 02 mov %eax,(%rdx)
> 35: c3 retq
>
>
This driver, mainly used for ARM
For ARM, situation a little bit different:
$ arm-linux-gnueabihf-gcc -c -O2 -DSWITCH foo.c
$ size foo.o
text data bss dec hex filename
32 0 0 32 20 foo.o
$ arm-linux-gnueabihf-objdump -d foo.o
foo.o: file format elf32-littlearm
Disassembly of section .text:
00000000 <foo>:
0: 6803 ldr r3, [r0, #0]
2: b143 cbz r3, 16 <foo+0x16>
4: f3c1 4102 ubfx r1, r1, #16, #3
8: 2901 cmp r1, #1
a: d005 beq.n 18 <foo+0x18>
c: 2902 cmp r1, #2
e: d000 beq.n 12 <foo+0x12>
10: 4770 bx lr
12: 6883 ldr r3, [r0, #8]
14: 6013 str r3, [r2, #0]
16: 4770 bx lr
18: 6843 ldr r3, [r0, #4]
1a: 6013 str r3, [r2, #0]
1c: 4770 bx lr
1e: bf00 nop
$ arm-linux-gnueabihf-gcc -c -O2 foo.c
$ size foo.o
text data bss dec hex filename
28 0 0 28 1c foo.o
$ arm-linux-gnueabihf-objdump -d foo.o
foo.o: file format elf32-littlearm
Disassembly of section .text:
00000000 <foo>:
0: 6803 ldr r3, [r0, #0]
2: b13b cbz r3, 14 <foo+0x14>
4: f3c1 4102 ubfx r1, r1, #16, #3
8: 2901 cmp r1, #1
a: d004 beq.n 16 <foo+0x16>
c: 2902 cmp r1, #2
e: bf04 itt eq
10: 6883 ldreq r3, [r0, #8]
12: 6013 streq r3, [r2, #0]
14: 4770 bx lr
16: 6843 ldr r3, [r0, #4]
18: 6013 str r3, [r2, #0]
1a: 4770 bx lr
So, it's shorter.
--
Regards,
Ivan Khoronzhuk
next prev parent reply other threads:[~2018-07-27 20:23 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-27 19:13 [PATCH net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions Ivan Khoronzhuk
2018-07-27 19:15 ` Grygorii Strashko
2018-07-27 19:15 ` Grygorii Strashko
2018-07-27 19:21 ` Joe Perches
2018-07-27 19:36 ` Ivan Khoronzhuk
2018-07-27 19:52 ` Ivan Khoronzhuk
2018-07-27 20:04 ` Joe Perches
2018-07-27 20:23 ` Ivan Khoronzhuk [this message]
2018-07-27 20:29 ` Joe Perches
2018-07-27 20:32 ` Andrew Lunn
2018-07-27 20:49 ` Ivan Khoronzhuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180727202348.GC2619@khorivan \
--to=ivan.khoronzhuk@linaro.org \
--cc=davem@davemloft.net \
--cc=grygorii.strashko@ti.com \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.