Re: 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 [off-list ref] wrote:

quoted

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

On Tue, 30 Dec 2003 12:43:21 -0500
Jeff Garzik [off-list ref] wrote:

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.

Ok, this is reasonable and works.

Though, is there any particular reason you don't like adding a
"|| irqs_disabled()" check to the if statement instead?
I prefer that solution better actually.

On Thu, Jan 01, 2004 at 12:42:18PM -0800, David S. Miller wrote:

On Tue, 30 Dec 2003 12:43:21 -0500
Jeff Garzik [off-list ref] wrote:

quoted

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.

Ok, this is reasonable and works.

Though, is there any particular reason you don't like adding a
"|| irqs_disabled()" check to the if statement instead?
I prefer that solution better actually.

Yep, in fact when I wrote the above message, I came across a couple when I
was pondering...
* the destructor runs in a more predictable context.
* given the problem that started this thread, the 'if' test is a
  potentially problematic area.  Why not eliminate all possibility that
  this problem will occur again?

The only counter argument to this -- to which I have no data to answer --
is that there may be advantage to calling __kfree_skb immediately
instead of deferring it slightly.  I didn't think that disadvantage
outweighted the above, but who knows...  I can possibly be convinced
otherwise.  (and "otherwise" would be using || irqs_disabled())

For the users who don't know/don't care about their context, it just
seemed to me that they were not a hot path like users of dev_kfree_skb()
and dev_kfree_skb_irq() [unconditional] are...

	Jeff

On Thu, 1 Jan 2004 21:58:07 -0500
Jeff Garzik [off-list ref] wrote:

On Thu, Jan 01, 2004 at 12:42:18PM -0800, David S. Miller wrote:

quoted

Though, is there any particular reason you don't like adding a
"|| irqs_disabled()" check to the if statement instead?
I prefer that solution better actually.

Yep, in fact when I wrote the above message, I came across a couple when I
was pondering...
* the destructor runs in a more predictable context.
* given the problem that started this thread, the 'if' test is a
  potentially problematic area.  Why not eliminate all possibility that
  this problem will occur again?

The way I see this, dev_kfree_skb_any() is not used in any performance critical
path, so at worst during device shutdown, reset, or power-down, TX queue
packet freeing work could be delayed by up to one jiffie.

Therefore I've put the "|| irqs_disabled()" version of the fix into my tree.

Thanks for working this out with me Jeff :)

On Thu, 1 Jan 2004 21:58:07 -0500
Jeff Garzik [off-list ref] wrote:

On Thu, Jan 01, 2004 at 12:42:18PM -0800, David S. Miller wrote:

quoted

Though, is there any particular reason you don't like adding a
"|| irqs_disabled()" check to the if statement instead?
I prefer that solution better actually.

Yep, in fact when I wrote the above message, I came across a couple when I
was pondering...
* the destructor runs in a more predictable context.
* given the problem that started this thread, the 'if' test is a
  potentially problematic area.  Why not eliminate all possibility that
  this problem will occur again?

The way I see this, dev_kfree_skb_any() is not used in any performance critical
path, so at worst during device shutdown, reset, or power-down, TX queue
packet freeing work could be delayed by up to one jiffie.

Therefore I've put the "|| irqs_disabled()" version of the fix into my tree.

Thanks for working this out with me Jeff :)