Michael Buesch wrote:
> On Saturday 12 September 2009 18:41:12 Oliver Hartkopp wrote:
>> Michael Buesch wrote:
>>
>>>> As there are several users in the kernel do exact this test and call the
>>>> appropriate netif_rx() function, i would suggest to create a static inline
>>>> function:
>>>>
>>>> static inline int netif_rx_ti(struct sk_buff *skb)
>>>> {
>>>> if (in_interrupt())
>>>> return netif_rx(skb);
>>>> return netif_rx_ni(skb);
>>>> }
>>>>
>>>> ('ti' for test in_interrupt())
>>>>
>>>> in include/linux/netdevice.h
>>>>
>>>> What do you think about that?
>>> Yeah, I'm fine with that.
>>>
>> Hi Michael,
>>
>> i cooked a patch that introduces netif_rx_ti() and fixes up the problems in
>> mac80211 and the CAN subsystem.
>>
>> Currently i'm pondering whether netif_rx_ti() is needed in all cases or if
>> there are code sections that'll never be executed from irq-context.
>>
>> In theses cases netif_rx_ni() should be prefered to netif_rx_ti() to prevent
>> the obsolete check ...
>>
>> Is there any of your changes that should better use netif_rx_ni() ?
>>
>> Regards,
>> Oliver
>>
>
> Well, I'd say this check does not cost much at all.
> If I were the net maintainer, I'd get rid of netif_rx_ni() _and_ netif_rx_ti() and
> do the check internally in netif_rx().
> But as I don't have to decide that, I just want the mac80211 issue fixed.
>
Like this?
int netif_rx(struct sk_buff *skb)
{
int err;
if (likely(in_interrupt()))
err = __netif_rx(skb);
else {
preempt_disable();
err = __netif_rx(skb);
if (local_softirq_pending())
do_softirq();
preempt_enable();
}
return err;
}
I don't know how expensive in_interrupt() is ... checking the code does not
give any answers to *me* ;-)
But i found
356 netif_rx()
but only
18 netif_rx_ni()
in the kernel tree.
And three of them check for in_interrupt() before using netif_rx() or
netif_rx_ni() ...
Finally i would tend to introduce netif_rx_ti() in include/linux/netdevice.h
as described above, for the rare code that can be used inside and outside the
irq context.
I assume the affected code in the CAN stuff has to use netif_rx_ni() - but i
will doublecheck that (and prepare a separate CAN patch).
For the mac80211 i would suggest to check whether you really need
netif_rx()/netif_rx_ni()/netif_rx_ti() in all the regarded cases.
I assume always using netif_rx_ti() (as you proposed in the original patch) is
not the most efficient approach.
Best regards,
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
