Message ID | 1430998870-1453-1-git-send-email-dvlasenk@redhat.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 05/07/2015 04:41 AM, Denys Vlasenko wrote: > These functions compile to ~60 bytes of machine code each. > > With this .config: http://busybox.net/~vda/kernel_config > there are 617 calls to netif_tx_stop_queue() > and 49 calls to netif_tx_stop_all_queues() in vmlinux. > > Code size is reduced by 27 kbytes: > > text data bss dec hex filename > 82426986 22255416 20627456 125309858 77813a2 vmlinux.before > 82399481 22255416 20627456 125282353 777a831 vmlinux > > It may seem strange that a seemingly simple code like one in > netif_tx_stop_queue() compiles to ~60 bytes of code. > Well, it's true. Here's its disassembly: > > netif_tx_stop_queue: > e8 b0 15 4d 00 callq <__fentry__> This bit was added because you converted this to a function. > 48 85 ff test %rdi,%rdi > 75 25 jne <netif_tx_stop_queue+0x2f> This bit is your WARN_ON test > 55 push %rbp > be 7a 18 00 00 mov $0x187a,%esi > 48 c7 c7 50 59 d8 85 mov $.rodata+0x1d85950,%rdi > 48 89 e5 mov %rsp,%rbp > e8 54 5a 7d fd callq <warn_slowpath_null> > 48 c7 c7 5f 59 d8 85 mov $.rodata+0x1d8595f,%rdi > 31 c0 xor %eax,%eax > e8 b0 47 48 00 callq <printk> > eb 09 jmp <netif_tx_stop_queue+0x38> This is the WARN_ON action. One thing you might try doing is moving this to a function of its own instead of moving the entire thing out of being an inline. You may find you still get most of the space savings as I wonder if the string for the printk isn't being duplicated for each caller. > f0 80 8f e0 01 00 00 01 lock orb $0x1,0x1e0(%rdi) This is your set bit operation. If you were to drop the whole WARN_ON then this is the only thing you would be inlining. That is only 8 bytes in size which would probably be comparable to the callq and register sorting needed for a function call. > c3 retq > 5d pop %rbp > c3 retq The rest of this is just more function overhead, one return for your standard path, and a pop and a return for the WARN_ON path. > > This causes gcc to auto-deinline it before this patch, but with 203 separate > copies in each module which uses this function: > > $ nm --size-sort vmlinux.before | grep -e ' netif_tx_stop_queue$' | wc -l > 203 > > Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com> > CC: David S. Miller <davem@davemloft.net> > CC: Jiri Pirko <jpirko@redhat.com> > CC: linux-kernel@vger.kernel.org > CC: netdev@vger.kernel.org > CC: netfilter-devel@vger.kernel.org > --- Have you done any performance testing on this change? I suspect there will likely be a noticeable impact some some tests. > include/linux/netdevice.h | 19 ++----------------- > net/core/dev.c | 21 +++++++++++++++++++++ > 2 files changed, 23 insertions(+), 17 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index dcf6ec2..f650d16 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -2546,14 +2546,7 @@ static inline void netif_tx_wake_all_queues(struct net_device *dev) > } > } > > -static inline void netif_tx_stop_queue(struct netdev_queue *dev_queue) > -{ > - if (WARN_ON(!dev_queue)) { > - pr_info("netif_stop_queue() cannot be called before register_netdev()\n"); > - return; > - } > - set_bit(__QUEUE_STATE_DRV_XOFF, &dev_queue->state); > -} > +void netif_tx_stop_queue(struct netdev_queue *dev_queue); It looks to me like most of the overhead for this function is the WARN_ON. Without that function would just be the "lock orb". The question I would have is why do we need the WARN_ON? Why not let any drivers that call netif_stop_queue before the netdev is registered take the NULL pointer dereference? The would likely learn real quick not to do that and a NULL pointer deference is fairly easy to debug. You could probably even just replace the WARN_ON with a comment that if you get a NULL pointer dereference here you probably called it before register_netdev. > > /** > * netif_stop_queue - stop transmitted packets > @@ -2567,15 +2560,7 @@ static inline void netif_stop_queue(struct net_device *dev) > netif_tx_stop_queue(netdev_get_tx_queue(dev, 0)); > } > > -static inline void netif_tx_stop_all_queues(struct net_device *dev) > -{ > - unsigned int i; > - > - for (i = 0; i < dev->num_tx_queues; i++) { > - struct netdev_queue *txq = netdev_get_tx_queue(dev, i); > - netif_tx_stop_queue(txq); > - } > -} > +void netif_tx_stop_all_queues(struct net_device *dev); > > static inline bool netif_tx_queue_stopped(const struct netdev_queue *dev_queue) > { This is usually slow path for most device drivers so it should fine to uninline. > diff --git a/net/core/dev.c b/net/core/dev.c > index 962ee9d..569031f 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6261,6 +6261,27 @@ static int netif_alloc_netdev_queues(struct net_device *dev) > return 0; > } > > +void netif_tx_stop_queue(struct netdev_queue *dev_queue) > +{ > + if (WARN_ON(!dev_queue)) { > + pr_info("netif_stop_queue() cannot be called before register_netdev()\n"); > + return; > + } > + set_bit(__QUEUE_STATE_DRV_XOFF, &dev_queue->state); > +} > +EXPORT_SYMBOL(netif_tx_stop_queue); > + One thing I noticed on reviewing the assembly above was that you should probably wrap the !dev_queue check in an unlikely. It would save you some unnecessary jumps instructions. > +void netif_tx_stop_all_queues(struct net_device *dev) > +{ > + unsigned int i; > + > + for (i = 0; i < dev->num_tx_queues; i++) { > + struct netdev_queue *txq = netdev_get_tx_queue(dev, i); > + netif_tx_stop_queue(txq); > + } > +} > +EXPORT_SYMBOL(netif_tx_stop_all_queues); > + > /** > * register_netdevice - register a network device > * @dev: device to register -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-05-07 at 10:14 -0700, Alexander Duyck wrote: > On 05/07/2015 04:41 AM, Denys Vlasenko wrote: > > These functions compile to ~60 bytes of machine code each. > > > > With this .config: http://busybox.net/~vda/kernel_config > > there are 617 calls to netif_tx_stop_queue() > > and 49 calls to netif_tx_stop_all_queues() in vmlinux. > > > > Code size is reduced by 27 kbytes: [] > This is the WARN_ON action. One thing you might try doing is moving > this to a function of its own instead of moving the entire thing out of > being an inline. You may find you still get most of the space savings > as I wonder if the string for the printk isn't being duplicated for each > caller. It is effectively duplicated (with different prefixes) if there is a #define pr_fmt(fmt) "some_prefix: " fmt before this code is reached. That's most callers now. The code that doesn't have a pr_fmt should get symbol deduplicated at link time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/2015 07:14 PM, Alexander Duyck wrote: > On 05/07/2015 04:41 AM, Denys Vlasenko wrote: >> These functions compile to ~60 bytes of machine code each. >> >> With this .config: http://busybox.net/~vda/kernel_config >> there are 617 calls to netif_tx_stop_queue() >> and 49 calls to netif_tx_stop_all_queues() in vmlinux. >> >> Code size is reduced by 27 kbytes: >> >> text data bss dec hex filename >> 82426986 22255416 20627456 125309858 77813a2 vmlinux.before >> 82399481 22255416 20627456 125282353 777a831 vmlinux >> >> It may seem strange that a seemingly simple code like one in >> netif_tx_stop_queue() compiles to ~60 bytes of code. >> Well, it's true. Here's its disassembly: >> >> netif_tx_stop_queue: ... >> 55 push %rbp >> be 7a 18 00 00 mov $0x187a,%esi >> 48 c7 c7 50 59 d8 85 mov $.rodata+0x1d85950,%rdi >> 48 89 e5 mov %rsp,%rbp >> e8 54 5a 7d fd callq <warn_slowpath_null> >> 48 c7 c7 5f 59 d8 85 mov $.rodata+0x1d8595f,%rdi >> 31 c0 xor %eax,%eax >> e8 b0 47 48 00 callq <printk> >> eb 09 jmp <netif_tx_stop_queue+0x38> > > This is the WARN_ON action. One thing you might try doing is moving > this to a function of its own instead of moving the entire thing > out of being an inline. If WARN_ON check would be moved into a function, the call overhead would still be there, while each callsite will be larder than with this patch. > You may find you still get most > of the space savings as I wonder if the string for the printk > isn't being duplicated for each caller. Yes, strings are duplicated: $ strings vmlinux0 | grep 'cannot be called before register_netdev' 6netif_stop_queue() cannot be called before register_netdev() 6tun: netif_stop_queue() cannot be called before register_netdev() 6cc770: netif_stop_queue() cannot be called before register_netdev() 63c589_cs: netif_stop_queue() cannot be called before register_netdev() 63c574_cs: netif_stop_queue() cannot be called before register_netdev() 6typhoon netif_stop_queue() cannot be called before register_netdev() 6axnet_cs: netif_stop_queue() cannot be called before register_netdev() 6pcnet_cs: netif_stop_queue() cannot be called before register_netdev() ... However, they amount only to ~5.7k out of 27k: $ strings vmlinux0 | grep 'cannot be called before register_netdev' | wc -c 5731 >> f0 80 8f e0 01 00 00 01 lock orb $0x1,0x1e0(%rdi) > > This is your set bit operation. If you were to drop the whole WARN_ON > then this is the only thing you would be inlining. It's up to networking people to decide. I would happily send a patch which drops WARN_ON if they say that's ok with them. Davem? > That is only 8 bytes in size which would probably be comparable to the callq > and register sorting needed for a function call. "lock or" in my tests takes 21 cycles even on exclusively cached L1 data cache line. Added "call+ret" is 4-5 cycles. > Have you done any performance testing on this change? No. > I suspect there will likely be a noticeable impact some some tests. (1) It's *transmit off* operation. Usually it means that we have to turn transmit off because hw TX queue is full. So the bottleneck is likely the network, not the CPU. (2) It was auto-deinlined by gcc anyway. We already were unknownigly using the uninlined version for some time. Apparently, it wasn't noticed.
On 05/08/2015 02:45 AM, Denys Vlasenko wrote: > On 05/07/2015 07:14 PM, Alexander Duyck wrote: >> On 05/07/2015 04:41 AM, Denys Vlasenko wrote: >>> These functions compile to ~60 bytes of machine code each. >>> >>> With this .config: http://busybox.net/~vda/kernel_config >>> there are 617 calls to netif_tx_stop_queue() >>> and 49 calls to netif_tx_stop_all_queues() in vmlinux. >>> >>> Code size is reduced by 27 kbytes: >>> >>> text data bss dec hex filename >>> 82426986 22255416 20627456 125309858 77813a2 vmlinux.before >>> 82399481 22255416 20627456 125282353 777a831 vmlinux >>> >>> It may seem strange that a seemingly simple code like one in >>> netif_tx_stop_queue() compiles to ~60 bytes of code. >>> Well, it's true. Here's its disassembly: >>> >>> netif_tx_stop_queue: > ... >>> 55 push %rbp >>> be 7a 18 00 00 mov $0x187a,%esi >>> 48 c7 c7 50 59 d8 85 mov $.rodata+0x1d85950,%rdi >>> 48 89 e5 mov %rsp,%rbp >>> e8 54 5a 7d fd callq <warn_slowpath_null> >>> 48 c7 c7 5f 59 d8 85 mov $.rodata+0x1d8595f,%rdi >>> 31 c0 xor %eax,%eax >>> e8 b0 47 48 00 callq <printk> >>> eb 09 jmp <netif_tx_stop_queue+0x38> >> This is the WARN_ON action. One thing you might try doing is moving >> this to a function of its own instead of moving the entire thing >> out of being an inline. > If WARN_ON check would be moved into a function, the call overhead > would still be there, while each callsite will be larder than with > this patch. > >> You may find you still get most >> of the space savings as I wonder if the string for the printk >> isn't being duplicated for each caller. > Yes, strings are duplicated: > > $ strings vmlinux0 | grep 'cannot be called before register_netdev' > 6netif_stop_queue() cannot be called before register_netdev() > 6tun: netif_stop_queue() cannot be called before register_netdev() > 6cc770: netif_stop_queue() cannot be called before register_netdev() > 63c589_cs: netif_stop_queue() cannot be called before register_netdev() > 63c574_cs: netif_stop_queue() cannot be called before register_netdev() > 6typhoon netif_stop_queue() cannot be called before register_netdev() > 6axnet_cs: netif_stop_queue() cannot be called before register_netdev() > 6pcnet_cs: netif_stop_queue() cannot be called before register_netdev() > ... > > However, they amount only to ~5.7k out of 27k: > > $ strings vmlinux0 | grep 'cannot be called before register_netdev' | wc -c > 5731 > Yeah, they are probably coalesced per .c file since the compiler cannot span files. The average driver probably calls it 2 or more times which is why it is only about 1/5 instead of 1/2 of the total bytecount. Also the count above excludes carriage returns and NULL characters. >>> f0 80 8f e0 01 00 00 01 lock orb $0x1,0x1e0(%rdi) >> This is your set bit operation. If you were to drop the whole WARN_ON >> then this is the only thing you would be inlining. > It's up to networking people to decide. I would happily send a patch which drops > WARN_ON if they say that's ok with them. Davem? This was added under commit 18543a643fae6 ("net: Detect and ignore netif_stop_queue() calls before register_netdev()"). I think the time for allowing drivers to ignore the WARN_ON has passed and at this point they should be strongly encouraged to fix the issue via a NULL pointer dereference if they still haven't gotten the issue resolved so we can track them down and fix them. I'd say add a comment here in case someone triggers this and does some debugging, but the WARN_ON at this point has proven it is too expensive. >> That is only 8 bytes in size which would probably be comparable to the callq >> and register sorting needed for a function call. > "lock or" in my tests takes 21 cycles even on exclusively cached > L1 data cache line. Added "call+ret" is 4-5 cycles. It is an expensive instruction, but pushing it out into a separate function just adds that much more overhead. >> Have you done any performance testing on this change? > No. The most likely thing to exercise this would probably be something like a standard pktgen test. It should be able to put enough stress on a single queue for the function to be called frequently. >> I suspect there will likely be a noticeable impact some some tests. > (1) It's *transmit off* operation. Usually it means that we have to turn > transmit off because hw TX queue is full. So the bottleneck is likely > the network, not the CPU. That is true. However there are still scenarios such as pktgen where we would be triggering this often and I would prefer to keep it as fast as possible since it is still kind of hard to maintain line rate 10Gb/s for some of my traffic generator setups. > > (2) It was auto-deinlined by gcc anyway. We already were unknownigly > using the uninlined version for some time. Apparently, it wasn't noticed. Depends on what cases where uninlined. I believe the compiler is making the decision per .c file so each driver is handling it differently. For example it looks like the ixgbe driver was still inlining this in my -O2 build, so that is one case that was not. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 08, 2015 at 08:50:37AM -0700, Alexander Duyck wrote: > > >>> f0 80 8f e0 01 00 00 01 lock orb $0x1,0x1e0(%rdi) > >>This is your set bit operation. If you were to drop the whole WARN_ON > >>then this is the only thing you would be inlining. > >It's up to networking people to decide. I would happily send a patch which drops > >WARN_ON if they say that's ok with them. Davem? > > This was added under commit 18543a643fae6 ("net: Detect and ignore > netif_stop_queue() calls before register_netdev()"). I think the time for > allowing drivers to ignore the WARN_ON has passed and at this point they > should be strongly encouraged to fix the issue via a NULL pointer > dereference if they still haven't gotten the issue resolved so we can track > them down and fix them. I'd say add a comment here in case someone triggers > this and does some debugging, but the WARN_ON at this point has proven it is > too expensive. +1 5 years for this WARN_ON was enough. Time to remove it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Denys Vlasenko <dvlasenk@redhat.com> Date: Thu, 7 May 2015 13:41:10 +0200 > These functions compile to ~60 bytes of machine code each. As others have suggested, just kill the WARN_ON(). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index dcf6ec2..f650d16 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2546,14 +2546,7 @@ static inline void netif_tx_wake_all_queues(struct net_device *dev) } } -static inline void netif_tx_stop_queue(struct netdev_queue *dev_queue) -{ - if (WARN_ON(!dev_queue)) { - pr_info("netif_stop_queue() cannot be called before register_netdev()\n"); - return; - } - set_bit(__QUEUE_STATE_DRV_XOFF, &dev_queue->state); -} +void netif_tx_stop_queue(struct netdev_queue *dev_queue); /** * netif_stop_queue - stop transmitted packets @@ -2567,15 +2560,7 @@ static inline void netif_stop_queue(struct net_device *dev) netif_tx_stop_queue(netdev_get_tx_queue(dev, 0)); } -static inline void netif_tx_stop_all_queues(struct net_device *dev) -{ - unsigned int i; - - for (i = 0; i < dev->num_tx_queues; i++) { - struct netdev_queue *txq = netdev_get_tx_queue(dev, i); - netif_tx_stop_queue(txq); - } -} +void netif_tx_stop_all_queues(struct net_device *dev); static inline bool netif_tx_queue_stopped(const struct netdev_queue *dev_queue) { diff --git a/net/core/dev.c b/net/core/dev.c index 962ee9d..569031f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6261,6 +6261,27 @@ static int netif_alloc_netdev_queues(struct net_device *dev) return 0; } +void netif_tx_stop_queue(struct netdev_queue *dev_queue) +{ + if (WARN_ON(!dev_queue)) { + pr_info("netif_stop_queue() cannot be called before register_netdev()\n"); + return; + } + set_bit(__QUEUE_STATE_DRV_XOFF, &dev_queue->state); +} +EXPORT_SYMBOL(netif_tx_stop_queue); + +void netif_tx_stop_all_queues(struct net_device *dev) +{ + unsigned int i; + + for (i = 0; i < dev->num_tx_queues; i++) { + struct netdev_queue *txq = netdev_get_tx_queue(dev, i); + netif_tx_stop_queue(txq); + } +} +EXPORT_SYMBOL(netif_tx_stop_all_queues); + /** * register_netdevice - register a network device * @dev: device to register
These functions compile to ~60 bytes of machine code each. With this .config: http://busybox.net/~vda/kernel_config there are 617 calls to netif_tx_stop_queue() and 49 calls to netif_tx_stop_all_queues() in vmlinux. Code size is reduced by 27 kbytes: text data bss dec hex filename 82426986 22255416 20627456 125309858 77813a2 vmlinux.before 82399481 22255416 20627456 125282353 777a831 vmlinux It may seem strange that a seemingly simple code like one in netif_tx_stop_queue() compiles to ~60 bytes of code. Well, it's true. Here's its disassembly: netif_tx_stop_queue: e8 b0 15 4d 00 callq <__fentry__> 48 85 ff test %rdi,%rdi 75 25 jne <netif_tx_stop_queue+0x2f> 55 push %rbp be 7a 18 00 00 mov $0x187a,%esi 48 c7 c7 50 59 d8 85 mov $.rodata+0x1d85950,%rdi 48 89 e5 mov %rsp,%rbp e8 54 5a 7d fd callq <warn_slowpath_null> 48 c7 c7 5f 59 d8 85 mov $.rodata+0x1d8595f,%rdi 31 c0 xor %eax,%eax e8 b0 47 48 00 callq <printk> eb 09 jmp <netif_tx_stop_queue+0x38> f0 80 8f e0 01 00 00 01 lock orb $0x1,0x1e0(%rdi) c3 retq 5d pop %rbp c3 retq This causes gcc to auto-deinline it before this patch, but with 203 separate copies in each module which uses this function: $ nm --size-sort vmlinux.before | grep -e ' netif_tx_stop_queue$' | wc -l 203 Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com> CC: David S. Miller <davem@davemloft.net> CC: Jiri Pirko <jpirko@redhat.com> CC: linux-kernel@vger.kernel.org CC: netdev@vger.kernel.org CC: netfilter-devel@vger.kernel.org --- include/linux/netdevice.h | 19 ++----------------- net/core/dev.c | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 17 deletions(-)