netdev - Re: [PATCH 0/3] URGENT for 3.9: net: fec: revert NAPI introduction

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]

Date:	Thu, 25 Apr 2013 22:45:53 +0800
From:	Frank Li <lznuaa@...il.com>
To:	Lucas Stach <l.stach@...gutronix.de>
Cc:	Fabio Estevam <festevam@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Frank Li <Frank.Li@...escale.com>,
	Shawn Guo <shawn.guo@...aro.org>
Subject: Re: [PATCH 0/3] URGENT for 3.9: net: fec: revert NAPI introduction

2013/4/25 Lucas Stach <l.stach@...gutronix.de>:
> Am Montag, den 22.04.2013, 17:17 +0800 schrieb Frank Li:
>> 2013/4/22 Lucas Stach <l.stach@...gutronix.de>:
>> > Hi all,
>> >
>> > Am Samstag, den 20.04.2013, 20:35 +0800 schrieb Frank Li:
>> >> 2013/4/20 Fabio Estevam <festevam@...il.com>
>> >> >
>> >> > Lucas,
>> >> >
>> >> > On Fri, Apr 19, 2013 at 11:36 AM, Lucas Stach <l.stach@...gutronix.de> wrote:
>> >> > > Those patches introduce instability to the point of kernel OOPSes with
>> >> > > NULL-ptr dereferences.
>> >> > >
>> >> > > The patches drop locks from the code without justifying why this would
>> >> > > be safe at all. In fact it isn't safe as now the controller restart can
>> >> > > happily free the RX and TX ring buffers while the NAPI poll function is
>> >> > > still accessing them. So with a heavily loaded but slightly instable
>> >>
>> >> I think a possible solution is disable NAPI in restart function.
>> >> So only one thread can reset BD queue.
>> >>
>> >> BD queue is nolock design.
>> >>
>> > It doesn't matter at all that the hardware BD queue is designed to be
>> > operated lockless, you still have to synchronize the driver functions to
>> > each other and explicit locks are a far better way to achieve this than
>> > some implicit tunneling through a single thread or other such things.
>>
>> Not hardware BD queue.
>> I redesign software BD queue as lockless queue.
>>
>> After put actual queue process work to NAPI,  interrupt handle will
>> not interrupt xmit and NAPI function again.
>>
>> There are just one entry xmit to push new data to bd queue.
>> One entry fec_enet_tx to pull old data from bd queue.
>>
>>                        HARD_TX_LOCK(dev, txq, cpu);
>>
>>                         if (!netif_xmit_stopped(txq)) {
>>                                 __this_cpu_inc(xmit_recursion);
>>                                 rc = dev_hard_start_xmit(skb, dev, txq);
>>                                 __this_cpu_dec(xmit_recursion);
>>                                 if (dev_xmit_complete(rc)) {
>>                                         HARD_TX_UNLOCK(dev, txq);
>>                                         goto out;
>>                                 }
>>                         }
>>                         HARD_TX_UNLOCK(dev, txq);
>>
>> Restart function will only called at suspend/resume, init, and speed change.
>> So risk should not in heave loading.
>>
>> The other reason of remove lock is that fix deadlock detected by kernel.
>
> While I agree that lockless queues and NAPI disable while doing FEC
> restart is the way to go for further development, I tried to implement
> that yesterday and it needs some bigger changes to the driver to split
> things up properly, otherwise we need a lot of ad-hoc hackery to check
> if NAPI is enabled, which seems really error prone.
>
> think such a change in the driver is not acceptable in the current state
> of the cycle. I for one will not submit this change, as I'm not sure at
> all that it won't regress in other situations.

NAPI is direction. Can you send me your detail test step? run what command?
So we can easily reproduce your problem.

>
> Regards,
> Lucas
> --
> Pengutronix e.K.                           | Lucas Stach                 |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
--
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