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

[Lucas Stach <l.stach@pengutronix.de>]

Am Montag, den 22.04.2013, 17:17 +0800 schrieb Frank Li:
> 2013/4/22 Lucas Stach <l.stach@pengutronix.de>:
> > Hi all,
> >
> > Am Samstag, den 20.04.2013, 20:35 +0800 schrieb Frank Li:
> >> 2013/4/20 Fabio Estevam <festevam@gmail.com>
> >> >
> >> > Lucas,
> >> >
> >> > On Fri, Apr 19, 2013 at 11:36 AM, Lucas Stach <l.stach@pengutronix.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.

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 |