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 <jgarzik@xxxxxxxxx> 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.
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
|