Re: mac80211: NOHZ: local_softirq_pending 08

Michael Buesch wrote:

On Saturday 12 September 2009 18:41:12 Oliver Hartkopp wrote:

quoted

Michael Buesch wrote:

quoted

quoted

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html