| Message ID | 1277337341.26161.18.camel@localhost |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi. On Thu, Jun 24, 2010 at 12:55:41AM +0100, Ben Hutchings wrote: > This avoids scheduling in atomic context and also means that IRQs > will only be deferred for relatively short periods of time. > > Previously discussed in: > http://article.gmane.org/gmane.linux.network/155024 > > Reported-by: Arne Nordmark <nordmark@mech.kth.se> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32] > --- > drivers/net/3c59x.c | 66 ++++++++++++++++++++++++++++++--------------------- > 1 files changed, 39 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c > index beddef9..f4a3fb1 100644 > --- a/drivers/net/3c59x.c > +++ b/drivers/net/3c59x.c > @@ -644,9 +644,15 @@ struct vortex_private { > u16 deferred; /* Resend these interrupts when we > * bale from the ISR */ > u16 io_size; /* Size of PCI region (for release_region) */ > - spinlock_t lock; /* Serialise access to device & its vortex_private */ > - struct mii_if_info mii; /* MII lib hooks/info */ > - int window; /* Register window */ > + > + /* Serialises access to hardware other than MII and variables below. > + * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */ > + spinlock_t lock; > + > + spinlock_t mii_lock; /* Serialises access to MII */ > + struct mii_if_info mii; /* MII lib hooks/info */ > + spinlock_t window_lock; /* Serialises access to windowed regs */ You should initialize the new locks properly with spin_lock_init(). > + int window; /* Register window */ > }; > > static void window_set(struct vortex_private *vp, int window) > @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window) > static u ## size \ > window_read ## size(struct vortex_private *vp, int window, int addr) \ > { \ > + unsigned long flags; \ > + u ## size ret; \ > + spin_lock_irqsave(&vp->window_lock, flags); \ > window_set(vp, window); \ > - return ioread ## size(vp->ioaddr + addr); \ > + ret = ioread ## size(vp->ioaddr + addr); \ > + spin_unlock_irqrestore(&vp->window_lock, flags); \ > + return ret; \ > } \ > static void \ > window_write ## size(struct vortex_private *vp, u ## size value, \ > int window, int addr) \ > { \ > + unsigned long flags; \ > + spin_lock_irqsave(&vp->window_lock, flags); \ > window_set(vp, window); \ > iowrite ## size(value, vp->ioaddr + addr); \ > + spin_unlock_irqrestore(&vp->window_lock, flags); \ > } This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many places where this is not necessary at all. For example during device probe and device open, window_read/window_write are called multiple times, each time disabling the interrupts. I'd suggest to have unlocked, locked and irqsave versions of window_read/window_write and use them in appropriate places. > DEFINE_WINDOW_IO(8) > DEFINE_WINDOW_IO(16) > @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data) > pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo); > } > > - disable_irq_lockdep(dev->irq); > media_status = window_read16(vp, 4, Wn4_Media); > switch (dev->if_port) { > case XCVR_10baseT: case XCVR_100baseTx: case XCVR_100baseFx: > @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data) > case XCVR_MII: case XCVR_NWAY: > { > ok = 1; > - /* Interrupts are already disabled */ > - spin_lock(&vp->lock); > vortex_check_media(dev, 0); > - spin_unlock(&vp->lock); > } > break; > default: /* Other media types handled by Tx timeouts. */ > @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data) > if (!ok) { > unsigned int config; > > + spin_lock_irq(&vp->lock); This can still happen every 5 seconds if the NIC has no link beat and medialock is not set. So what about defering this locked codepath to a workqueue, or moving the whole vortex_timer to a delayed workqueue? In this case we don't need to disable all the interrups on the cpu, we could still use disable_irq then. The rest looks quite good to me. Thanks, Steffen -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-06-24 at 14:05 +0200, Steffen Klassert wrote: > Hi. > > On Thu, Jun 24, 2010 at 12:55:41AM +0100, Ben Hutchings wrote: > > This avoids scheduling in atomic context and also means that IRQs > > will only be deferred for relatively short periods of time. > > > > Previously discussed in: > > http://article.gmane.org/gmane.linux.network/155024 > > > > Reported-by: Arne Nordmark <nordmark@mech.kth.se> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > Tested-by: Arne Nordmark <nordmark@mech.kth.se> [against 2.6.32] > > --- > > drivers/net/3c59x.c | 66 ++++++++++++++++++++++++++++++--------------------- > > 1 files changed, 39 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c > > index beddef9..f4a3fb1 100644 > > --- a/drivers/net/3c59x.c > > +++ b/drivers/net/3c59x.c > > @@ -644,9 +644,15 @@ struct vortex_private { > > u16 deferred; /* Resend these interrupts when we > > * bale from the ISR */ > > u16 io_size; /* Size of PCI region (for release_region) */ > > - spinlock_t lock; /* Serialise access to device & its vortex_private */ > > - struct mii_if_info mii; /* MII lib hooks/info */ > > - int window; /* Register window */ > > + > > + /* Serialises access to hardware other than MII and variables below. > > + * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */ > > + spinlock_t lock; > > + > > + spinlock_t mii_lock; /* Serialises access to MII */ > > + struct mii_if_info mii; /* MII lib hooks/info */ > > + spinlock_t window_lock; /* Serialises access to windowed regs */ > > You should initialize the new locks properly with spin_lock_init(). Oops, yes, obviously. > > + int window; /* Register window */ > > }; > > > > static void window_set(struct vortex_private *vp, int window) > > @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window) > > static u ## size \ > > window_read ## size(struct vortex_private *vp, int window, int addr) \ > > { \ > > + unsigned long flags; \ > > + u ## size ret; \ > > + spin_lock_irqsave(&vp->window_lock, flags); \ > > window_set(vp, window); \ > > - return ioread ## size(vp->ioaddr + addr); \ > > + ret = ioread ## size(vp->ioaddr + addr); \ > > + spin_unlock_irqrestore(&vp->window_lock, flags); \ > > + return ret; \ > > } \ > > static void \ > > window_write ## size(struct vortex_private *vp, u ## size value, \ > > int window, int addr) \ > > { \ > > + unsigned long flags; \ > > + spin_lock_irqsave(&vp->window_lock, flags); \ > > window_set(vp, window); \ > > iowrite ## size(value, vp->ioaddr + addr); \ > > + spin_unlock_irqrestore(&vp->window_lock, flags); \ > > } > > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many > places where this is not necessary at all. For example during device probe and > device open, window_read/window_write are called multiple times, each time > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave > versions of window_read/window_write and use them in appropriate places. So what? These are not speed-critical. The fast-path functions do acquire the lock just once. > > DEFINE_WINDOW_IO(8) > > DEFINE_WINDOW_IO(16) > > @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data) > > pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo); > > } > > > > - disable_irq_lockdep(dev->irq); > > media_status = window_read16(vp, 4, Wn4_Media); > > switch (dev->if_port) { > > case XCVR_10baseT: case XCVR_100baseTx: case XCVR_100baseFx: > > @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data) > > case XCVR_MII: case XCVR_NWAY: > > { > > ok = 1; > > - /* Interrupts are already disabled */ > > - spin_lock(&vp->lock); > > vortex_check_media(dev, 0); > > - spin_unlock(&vp->lock); > > } > > break; > > default: /* Other media types handled by Tx timeouts. */ > > @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data) > > if (!ok) { > > unsigned int config; > > > > + spin_lock_irq(&vp->lock); > > This can still happen every 5 seconds if the NIC has no link beat and > medialock is not set. So what about defering this locked codepath to > a workqueue, or moving the whole vortex_timer to a delayed workqueue? > In this case we don't need to disable all the interrups on the cpu, we > could still use disable_irq then. This locked section is now very short - 5 or 6 register read/writes and no delays. We might even be able to get away without locking here as the only software state this accesses is dev->if_port and I don't think it can race with anything except SIOCGIFMAP (which seems harmless). Ben. > The rest looks quite good to me. > > Thanks, > > Steffen >
On Thu, Jun 24, 2010 at 01:57:19PM +0100, Ben Hutchings wrote: > > > > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many > > places where this is not necessary at all. For example during device probe and > > device open, window_read/window_write are called multiple times, each time > > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave > > versions of window_read/window_write and use them in appropriate places. > > So what? These are not speed-critical. The fast-path functions do > acquire the lock just once. > The point is that we should not disable the interrupts if we don't need to do so. It is not speed critical for the 3c59x driver but disabling the interrupts should be avoided whenever possible. For example during device probe and device open we can't race against an interrupt handler because the device is not yet running. An example from vortex_probe1() is: for (i = 0; i < 6; i++) window_write8(vp, dev->dev_addr[i], 2, i); which expands to someting like: for (i = 0; i < 6; i++) { unsigned long flags; spin_lock_irqsave(&vp->window_lock, flags); window_set(vp, window); iowrite8(dev->dev_addr[i], vp->ioaddr + i); spin_unlock_irqrestore(&vp->window_lock, flags); return ret; } which is quite odd in a codepath that could simply do: for (i = 0; i < 6; i++) { window_set(vp, window); iowrite8(dev->dev_addr[i], vp->ioaddr + i); } > > This locked section is now very short - 5 or 6 register read/writes and > no delays. We might even be able to get away without locking here as > the only software state this accesses is dev->if_port and I don't think > it can race with anything except SIOCGIFMAP (which seems harmless). > Best would be, if we don't need to disable the interrupts on this cpu at all. But then we probaply need to disable the interupt line with disable_irq. That's why I suggested to move the timer to thread context. Steffen -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2010-06-24 at 16:00 +0200, Steffen Klassert wrote: > On Thu, Jun 24, 2010 at 01:57:19PM +0100, Ben Hutchings wrote: > > > > > > This adds a lot of calls to spin_lock_irqsave/spin_unlock_irqrestore to many > > > places where this is not necessary at all. For example during device probe and > > > device open, window_read/window_write are called multiple times, each time > > > disabling the interrupts. I'd suggest to have unlocked, locked and irqsave > > > versions of window_read/window_write and use them in appropriate places. > > > > So what? These are not speed-critical. The fast-path functions do > > acquire the lock just once. > > > > The point is that we should not disable the interrupts if we don't need to > do so. It is not speed critical for the 3c59x driver but disabling the > interrupts should be avoided whenever possible. For example during device > probe and device open we can't race against an interrupt handler because > the device is not yet running. > > An example from vortex_probe1() is: > > for (i = 0; i < 6; i++) > window_write8(vp, dev->dev_addr[i], 2, i); > > which expands to someting like: > > for (i = 0; i < 6; i++) { > unsigned long flags; > spin_lock_irqsave(&vp->window_lock, flags); > window_set(vp, window); > iowrite8(dev->dev_addr[i], vp->ioaddr + i); > spin_unlock_irqrestore(&vp->window_lock, flags); > return ret; > } [...] I still fail to see why this matters. The sfc driver which I look after in my day job also uses spin_lock_irqsave() for each CSR update, when this could be avoided in the initialisation path. None of the many customers using rt kernels has ever complained about this. It's just not an important issue. Ben.
On Fri, Jun 25, 2010 at 01:45:59AM +0100, Ben Hutchings wrote: > > > > The point is that we should not disable the interrupts if we don't need to > > do so. It is not speed critical for the 3c59x driver but disabling the > > interrupts should be avoided whenever possible. For example during device > > probe and device open we can't race against an interrupt handler because > > the device is not yet running. > > > > An example from vortex_probe1() is: > > > > for (i = 0; i < 6; i++) > > window_write8(vp, dev->dev_addr[i], 2, i); > > > > which expands to someting like: > > > > for (i = 0; i < 6; i++) { > > unsigned long flags; > > spin_lock_irqsave(&vp->window_lock, flags); > > window_set(vp, window); > > iowrite8(dev->dev_addr[i], vp->ioaddr + i); > > spin_unlock_irqrestore(&vp->window_lock, flags); > > return ret; > > } > [...] > > I still fail to see why this matters. > These locks are not needed, why you want to have them arround? It adds overhead, even if they are not in a fast-path function. And much more important, this gives a reader of the code the impression that these locks are needed. For somebody who tries to understand the code, it will be probaply not that easy to figure out which of these locks are needed and for what reason. > The sfc driver which I look after in my day job also uses > spin_lock_irqsave() for each CSR update, when this could be avoided in > the initialisation path. None of the many customers using rt kernels > has ever complained about this. It's just not an important issue. Perhaps I'm fussy, personally I prefer to use the appropriate lock in any case. But that's just my opinion, other people might think different about that. Steffen -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Steffen Klassert <steffen.klassert@secunet.com> Date: Fri, 25 Jun 2010 10:24:47 +0200 > These locks are not needed, why you want to have them arround? Steffen I think you are being overly picky of Ben's changes. I'd rather have too much locking during device probe and initialization than a subtle bug that occurs because later on someone decides to move IRQ enabling earlier in the chip init path and now we get strange hangs that take forever to diagnose. I mean, extra locking in probe/init paths... ugh, there are so many more important things to worry about! Once Ben posts a new version of this second patch with the proper spin_lock_init() calls added I am going to apply both of his changes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 28, 2010 at 11:18:12PM -0700, David Miller wrote: > > Once Ben posts a new version of this second patch with the > proper spin_lock_init() calls added I am going to apply both > of his changes. Yes, of course apply them. It was just a recommendation to avoid the locks in the cases they are not needed. These patches are a real improvement, so I'm fine with them. Steffen -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/3c59x.c b/drivers/net/3c59x.c index beddef9..f4a3fb1 100644 --- a/drivers/net/3c59x.c +++ b/drivers/net/3c59x.c @@ -644,9 +644,15 @@ struct vortex_private { u16 deferred; /* Resend these interrupts when we * bale from the ISR */ u16 io_size; /* Size of PCI region (for release_region) */ - spinlock_t lock; /* Serialise access to device & its vortex_private */ - struct mii_if_info mii; /* MII lib hooks/info */ - int window; /* Register window */ + + /* Serialises access to hardware other than MII and variables below. + * The lock hierarchy is rtnl_lock > lock > mii_lock > window_lock. */ + spinlock_t lock; + + spinlock_t mii_lock; /* Serialises access to MII */ + struct mii_if_info mii; /* MII lib hooks/info */ + spinlock_t window_lock; /* Serialises access to windowed regs */ + int window; /* Register window */ }; static void window_set(struct vortex_private *vp, int window) @@ -661,15 +667,23 @@ static void window_set(struct vortex_private *vp, int window) static u ## size \ window_read ## size(struct vortex_private *vp, int window, int addr) \ { \ + unsigned long flags; \ + u ## size ret; \ + spin_lock_irqsave(&vp->window_lock, flags); \ window_set(vp, window); \ - return ioread ## size(vp->ioaddr + addr); \ + ret = ioread ## size(vp->ioaddr + addr); \ + spin_unlock_irqrestore(&vp->window_lock, flags); \ + return ret; \ } \ static void \ window_write ## size(struct vortex_private *vp, u ## size value, \ int window, int addr) \ { \ + unsigned long flags; \ + spin_lock_irqsave(&vp->window_lock, flags); \ window_set(vp, window); \ iowrite ## size(value, vp->ioaddr + addr); \ + spin_unlock_irqrestore(&vp->window_lock, flags); \ } DEFINE_WINDOW_IO(8) DEFINE_WINDOW_IO(16) @@ -1784,7 +1798,6 @@ vortex_timer(unsigned long data) pr_debug("dev->watchdog_timeo=%d\n", dev->watchdog_timeo); } - disable_irq_lockdep(dev->irq); media_status = window_read16(vp, 4, Wn4_Media); switch (dev->if_port) { case XCVR_10baseT: case XCVR_100baseTx: case XCVR_100baseFx: @@ -1805,10 +1818,7 @@ vortex_timer(unsigned long data) case XCVR_MII: case XCVR_NWAY: { ok = 1; - /* Interrupts are already disabled */ - spin_lock(&vp->lock); vortex_check_media(dev, 0); - spin_unlock(&vp->lock); } break; default: /* Other media types handled by Tx timeouts. */ @@ -1827,6 +1837,8 @@ vortex_timer(unsigned long data) if (!ok) { unsigned int config; + spin_lock_irq(&vp->lock); + do { dev->if_port = media_tbl[dev->if_port].next; } while ( ! (vp->available_media & media_tbl[dev->if_port].mask)); @@ -1855,6 +1867,8 @@ vortex_timer(unsigned long data) if (vortex_debug > 1) pr_debug("wrote 0x%08x to Wn3_Config\n", config); /* AKPM: FIXME: Should reset Rx & Tx here. P60 of 3c90xc.pdf */ + + spin_unlock_irq(&vp->lock); } leave_media_alone: @@ -1862,7 +1876,6 @@ leave_media_alone: pr_debug("%s: Media selection timer finished, %s.\n", dev->name, media_tbl[dev->if_port].name); - enable_irq_lockdep(dev->irq); mod_timer(&vp->timer, RUN_AT(next_tick)); if (vp->deferred) iowrite16(FakeIntr, ioaddr + EL3_CMD); @@ -2051,9 +2064,11 @@ vortex_start_xmit(struct sk_buff *skb, struct net_device *dev) int len = (skb->len + 3) & ~3; vp->tx_skb_dma = pci_map_single(VORTEX_PCI(vp), skb->data, len, PCI_DMA_TODEVICE); + spin_lock_irq(&vp->window_lock); window_set(vp, 7); iowrite32(vp->tx_skb_dma, ioaddr + Wn7_MasterAddr); iowrite16(len, ioaddr + Wn7_MasterLen); + spin_unlock_irq(&vp->window_lock); vp->tx_skb = skb; iowrite16(StartDMADown, ioaddr + EL3_CMD); /* netif_wake_queue() will be called at the DMADone interrupt. */ @@ -2225,6 +2240,7 @@ vortex_interrupt(int irq, void *dev_id) pr_debug("%s: interrupt, status %4.4x, latency %d ticks.\n", dev->name, status, ioread8(ioaddr + Timer)); + spin_lock(&vp->window_lock); window_set(vp, 7); do { @@ -2285,6 +2301,8 @@ vortex_interrupt(int irq, void *dev_id) iowrite16(AckIntr | IntReq | IntLatch, ioaddr + EL3_CMD); } while ((status = ioread16(ioaddr + EL3_STATUS)) & (IntLatch | RxComplete)); + spin_unlock(&vp->window_lock); + if (vortex_debug > 4) pr_debug("%s: exiting interrupt, status %4.4x.\n", dev->name, status); @@ -2806,37 +2824,22 @@ static void update_stats(void __iomem *ioaddr, struct net_device *dev) static int vortex_nway_reset(struct net_device *dev) { struct vortex_private *vp = netdev_priv(dev); - unsigned long flags; - int rc; - spin_lock_irqsave(&vp->lock, flags); - rc = mii_nway_restart(&vp->mii); - spin_unlock_irqrestore(&vp->lock, flags); - return rc; + return mii_nway_restart(&vp->mii); } static int vortex_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) { struct vortex_private *vp = netdev_priv(dev); - unsigned long flags; - int rc; - spin_lock_irqsave(&vp->lock, flags); - rc = mii_ethtool_gset(&vp->mii, cmd); - spin_unlock_irqrestore(&vp->lock, flags); - return rc; + return mii_ethtool_gset(&vp->mii, cmd); } static int vortex_set_settings(struct net_device *dev, struct ethtool_cmd *cmd) { struct vortex_private *vp = netdev_priv(dev); - unsigned long flags; - int rc; - spin_lock_irqsave(&vp->lock, flags); - rc = mii_ethtool_sset(&vp->mii, cmd); - spin_unlock_irqrestore(&vp->lock, flags); - return rc; + return mii_ethtool_sset(&vp->mii, cmd); } static u32 vortex_get_msglevel(struct net_device *dev) @@ -3059,6 +3062,8 @@ static int mdio_read(struct net_device *dev, int phy_id, int location) int read_cmd = (0xf6 << 10) | (phy_id << 5) | location; unsigned int retval = 0; + spin_lock_bh(&vp->mii_lock); + if (mii_preamble_required) mdio_sync(vp, 32); @@ -3082,6 +3087,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location) 4, Wn4_PhysicalMgmt); mdio_delay(vp); } + + spin_unlock_bh(&vp->mii_lock); + return retval & 0x20000 ? 0xffff : retval>>1 & 0xffff; } @@ -3091,6 +3099,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val int write_cmd = 0x50020000 | (phy_id << 23) | (location << 18) | value; int i; + spin_lock_bh(&vp->mii_lock); + if (mii_preamble_required) mdio_sync(vp, 32); @@ -3111,6 +3121,8 @@ static void mdio_write(struct net_device *dev, int phy_id, int location, int val 4, Wn4_PhysicalMgmt); mdio_delay(vp); } + + spin_unlock_bh(&vp->mii_lock); } /* ACPI: Advanced Configuration and Power Interface. */