On Tue, 2006-06-06 at 09:52 -0400, Neil Horman wrote: > I've been speaking about this fix with a Jeff Moyer, and we've come up with > some > concerns regarding its implementation. Specifically the call to > adapter->clean_rx in the case of the e1000 driver is rather a layering > violation in the netpoll code, in the sense that the function pointed to by > clean_rx > is functionality that is nominally used by the dev->poll method. In fact in > this case, it would appear possible since dev->poll is called under the > poll_lock, but dev->poll_controller is not, that is is possible to have cpus > in > a system executing in e1000_clean_rx_irq_[ps] at the same time leading to data > corruption: > > CPU0: > netpoll_poll_dev > dev->poll_controller (e1000_netpoll) > adapter->clean_rx (e1000_clean_rx_irq) > > CPU1: > napi_poll > dev->poll (e1000_clean) > e1000_clean_rx_irq
Hmmm. You may have a point. I don't think a spinlock is required, but we do need to check if the poll is already scheduled on another CPU, like netpoll does in poll_napi(). In practice, of course, we never see this. The only netpoll client in the kernel is netconsole, which doesn't do receives. A few Major Distros use netdump, which does do receives, but only after the system has crashed. In that case, all other CPUs are stopped anyway. However, just for the sake of correctness (and paranoia), I'll whip up another patch that does this check. Jeff, please do not commit this patch. -Mitch NB: The root of this problem is that e1000 uses a dummy netdev to schedule NAPI polling. Since netpoll doesn't know about the dummy netdev, it checks the "real" e1000 netdev struct to see if it's scheduled for polling. Since this is never the case, netpoll fails when e1000 is configured to use NAPI. Hence, this patch. Why is the dummy netdev in place? To support multi-queue RX. Our PCIe adapters support this, but the kernel's not _quite_ ready yet. Hopefully, the VJ net channel stuff will enable this feature. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
