On Fri, 14 Aug 2009, Linus Torvalds wrote:
>
> But not inline in the code, though. So yeah, it has a memory footprint,
> but shouldn't have a cache footprint.
An example of this: stack usage. This is the code with BUG_ON():
skb_push:
push %rbp
mov %esi,%eax
add %esi,0x68(%rdi)
neg %rax
mov %rsp,%rbp
add 0xd8(%rdi),%rax
mov %rax,0xd8(%rdi)
cmp 0xd0(%rdi),%rax
jae 2f
ud2a
1: jmp 1b
2: leaveq
retq
and note how in the code, we're just jumping over something like four
bytes ("ud2" plus that silly endless loop-jump just to make gcc happy
are both 2 bytes on x86[-64]).
Here's the same code with that horrid skb_under_panic():
skb_push:
push %rbp
mov %esi,%eax
mov %rsp,%rbp
neg %rax
push %rbx
mov %rdi,%rbx
sub $0x8,%rsp
add 0xd8(%rdi),%rax
add %esi,0x68(%rdi)
mov %rax,0xd8(%rdi)
cmp 0xd0(%rdi),%rax
jae 1f
mov 0x8(%rbp),%rdx
callq skb_under_panic
1: mov 0xd8(%rbx),%rax
pop %r11
pop %rbx
leaveq
retq
and notice how it creates a stack frame due to the call (even when the
call is never done!) and the dead space is now 11 bytes.
(Total size of functions: 41 bytes vs 64 bytes)
So things like that do make a difference. It's surprisingly expensive
to add a function call t what otherwise could be a leaf function. And
BUG_ON() avoids that.
Of course, even at just 41 bytes, it's not _entirely_ obvious that
inlining is the right thing. Of those 41 bytes, four are just function
overhead, and the "call" is five bytes in the caller and a few bytes of
spills and argument setups, but inlining is still probably going to change
each call-site from ~10 bytes to ~35 bytes.
Linus
---
include/linux/skbuff.h | 4 ---
net/core/skbuff.c | 51 +----------------------------------------------
2 files changed, 2 insertions(+), 53 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f2c69a2..2af92b4 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -483,10 +483,6 @@ extern int skb_cow_data(struct sk_buff *skb, int tailbits,
extern int skb_pad(struct sk_buff *skb, int pad);
#define dev_kfree_skb(a) consume_skb(a)
#define dev_consume_skb(a) kfree_skb_clean(a)
-extern void skb_over_panic(struct sk_buff *skb, int len,
- void *here);
-extern void skb_under_panic(struct sk_buff *skb, int len,
- void *here);
extern int skb_append_datato_frags(struct sock *sk, struct sk_buff *skb,
int getfrag(void *from, char *to, int offset,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9e0597d..ae63473 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -103,51 +103,6 @@ static struct pipe_buf_operations sock_pipe_buf_ops = {
.get = sock_pipe_buf_get,
};
-/*
- * Keep out-of-line to prevent kernel bloat.
- * __builtin_return_address is not used because it is not always
- * reliable.
- */
-
-/**
- * skb_over_panic - private function
- * @skb: buffer
- * @sz: size
- * @here: address
- *
- * Out of line support code for skb_put(). Not user callable.
- */
-void skb_over_panic(struct sk_buff *skb, int sz, void *here)
-{
- printk(KERN_EMERG "skb_over_panic: text:%p len:%d put:%d head:%p "
- "data:%p tail:%#lx end:%#lx dev:%s\n",
- here, skb->len, sz, skb->head, skb->data,
- (unsigned long)skb->tail, (unsigned long)skb->end,
- skb->dev ? skb->dev->name : "<NULL>");
- BUG();
-}
-EXPORT_SYMBOL(skb_over_panic);
-
-/**
- * skb_under_panic - private function
- * @skb: buffer
- * @sz: size
- * @here: address
- *
- * Out of line support code for skb_push(). Not user callable.
- */
-
-void skb_under_panic(struct sk_buff *skb, int sz, void *here)
-{
- printk(KERN_EMERG "skb_under_panic: text:%p len:%d put:%d head:%p "
- "data:%p tail:%#lx end:%#lx dev:%s\n",
- here, skb->len, sz, skb->head, skb->data,
- (unsigned long)skb->tail, (unsigned long)skb->end,
- skb->dev ? skb->dev->name : "<NULL>");
- BUG();
-}
-EXPORT_SYMBOL(skb_under_panic);
-
/* Allocate a new skbuff. We do this ourselves so we can fill in a few
* 'private' fields and also do memory statistics to find all the
* [BEEP] leaks.
@@ -1014,8 +969,7 @@ unsigned char *skb_put(struct sk_buff *skb, unsigned int len)
SKB_LINEAR_ASSERT(skb);
skb->tail += len;
skb->len += len;
- if (unlikely(skb->tail > skb->end))
- skb_over_panic(skb, len, __builtin_return_address(0));
+ BUG_ON(skb->tail > skb->end);
return tmp;
}
EXPORT_SYMBOL(skb_put);
@@ -1033,8 +987,7 @@ unsigned char *skb_push(struct sk_buff *skb, unsigned int len)
{
skb->data -= len;
skb->len += len;
- if (unlikely(skb->data<skb->head))
- skb_under_panic(skb, len, __builtin_return_address(0));
+ BUG_ON(skb->data < skb->head);
return skb->data;
}
EXPORT_SYMBOL(skb_push);
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
