netdev - Re: mac80211: NOHZ: local_softirq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4AABE34D.30401@hartkopp.net>
Date:	Sat, 12 Sep 2009 20:07:09 +0200
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	Michael Buesch <mb@...sch.de>
CC:	Kalle Valo <kalle.valo@....fi>, linux-wireless@...r.kernel.org,
	netdev@...r.kernel.org, Johannes Berg <johannes@...solutions.net>,
	"John W. Linville" <linville@...driver.com>
Subject: Re: mac80211: NOHZ: local_softirq_pending 08

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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html