lkml.org 
[lkml]   [2003]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: Problem with dev_kfree_skb_any() in 2.6.0
David S. Miller wrote:
> On Tue, 30 Dec 2003 01:12:21 -0500
> Jeff Garzik <jgarzik@pobox.com> wrote:
>
>
>>Think about the name of this function: dev_kfree_skb_any()
>>
>>If this function cannot be used -anywhere-, then the concept (and the
>>net stack) is fundamentally broken for this function. We must _remove_
>>the function, and thus _I_ have a lot of driver work to do.
>
>
> If it makes you happy, change the suffix of the name, I am
> not emotionally attached to the name.


It's not about the name itself. _Think_ about the name: what is the
purpose of the function? what does it imply? How have kernel hackers
used it in practice? I think you're focusing too closely on
implementation, rather than purpose.

I humbly request that we expend some brain CPU cycles to think about the
API here.

To review:
* The base requirement is that __kfree_skb shall not call the skb's
destructor in hard IRQ context.
* To that end, dev_kfree_skb_irq was created to queue skb's for
__kfree_skb'ing, when that requirement is not immediately satisfied.
* dev_kfree_skb_any was created for situations that either don't know,
or don't care, about the context in which skb's are freed. The function
name and implementation are less relevant than _purpose_.

As it stands now, dev_kfree_skb_any() does not serve the purpose for
which it is used in many drivers (not just the short list found in this
thread).

Luckily, I feel there is an easy solution, as shown in the attached
patch. We _already_ queue skbs in dev_kfree_skb_irq(). Therefore,
dev_kfree_skb_any() can simply use precisely that same solution. The
raise-softirq code will immediately proceed to action if we are not in
hard IRQ context, otherwise it will follow the expected path.

As an aside (tangent warning), we will need to consider further
queueing, if we are to follow the rest of the kernel: skb destructor
should really be in purely task context, i.e. make sure __kfree_skb does
its work in kernel thread context. But that's a discussion for another
day ;-)

Jeff

===== include/linux/netdevice.h 1.66 vs edited =====
--- 1.66/include/linux/netdevice.h Sat Nov 1 17:11:04 2003
+++ edited/include/linux/netdevice.h Tue Dec 30 12:29:40 2003
@@ -634,10 +634,7 @@
*/
static inline void dev_kfree_skb_any(struct sk_buff *skb)
{
- if (in_irq())
- dev_kfree_skb_irq(skb);
- else
- dev_kfree_skb(skb);
+ dev_kfree_skb_irq(skb);
}

#define HAVE_NETIF_RX 1
\
 
 \ /
  Last update: 2005-03-22 13:59    [W:0.033 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site