Konrad Rzeszutek Wilk
2012-Jan-12 14:17 UTC
Re: [PATCH] add netconsole support for xen-netfront
On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote:> add polling interface to xen-netfront device to support netconsole >Ian, any thoughts on the spinlock changes?> Signed-off-by: Tina.Yang <tina.yang@oracle.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Signed-off-by: Zhenzhong.Duan <zhenzhong.duan@oracle.com> > Tested-by: gurudas.pai <gurudas.pai@oracle.com> > --- > drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- > 1 files changed, 34 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index fa67905..db638b4 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -489,6 +489,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > int frags = skb_shinfo(skb)->nr_frags; > unsigned int offset = offset_in_page(data); > unsigned int len = skb_headlen(skb); > + unsigned long flags; > > frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); > if (unlikely(frags > MAX_SKB_FRAGS + 1)) { > @@ -498,12 +499,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > goto drop; > } > > - spin_lock_irq(&np->tx_lock); > + spin_lock_irqsave(&np->tx_lock, flags); > > if (unlikely(!netif_carrier_ok(dev) || > (frags > 1 && !xennet_can_sg(dev)) || > netif_needs_gso(skb, netif_skb_features(skb)))) { > - spin_unlock_irq(&np->tx_lock); > + spin_unlock_irqrestore(&np->tx_lock, flags); > goto drop; > } > > @@ -574,7 +575,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > if (!netfront_tx_slot_available(np)) > netif_stop_queue(dev); > > - spin_unlock_irq(&np->tx_lock); > + spin_unlock_irqrestore(&np->tx_lock, flags); > > return NETDEV_TX_OK; > > @@ -1228,6 +1229,33 @@ static int xennet_set_features(struct net_device *dev, > return 0; > } > > +static irqreturn_t xennet_interrupt(int irq, void *dev_id) > +{ > + struct net_device *dev = dev_id; > + struct netfront_info *np = netdev_priv(dev); > + unsigned long flags; > + > + spin_lock_irqsave(&np->tx_lock, flags); > + > + if (likely(netif_carrier_ok(dev))) { > + xennet_tx_buf_gc(dev); > + /* Under tx_lock: protects access to rx shared-ring indexes. */ > + if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > + napi_schedule(&np->napi); > + } > + > + spin_unlock_irqrestore(&np->tx_lock, flags); > + > + return IRQ_HANDLED; > +} > + > +#ifdef CONFIG_NET_POLL_CONTROLLER > +static void xennet_poll_controller(struct net_device *dev) > +{ > + xennet_interrupt(0, dev); > +} > +#endif > + > static const struct net_device_ops xennet_netdev_ops = { > .ndo_open = xennet_open, > .ndo_uninit = xennet_uninit, > @@ -1239,6 +1267,9 @@ static const struct net_device_ops xennet_netdev_ops = { > .ndo_validate_addr = eth_validate_addr, > .ndo_fix_features = xennet_fix_features, > .ndo_set_features = xennet_set_features, > +#ifdef CONFIG_NET_POLL_CONTROLLER > + .ndo_poll_controller = xennet_poll_controller, > +#endif > }; > > static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) > @@ -1448,26 +1479,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) > return 0; > } > > -static irqreturn_t xennet_interrupt(int irq, void *dev_id) > -{ > - struct net_device *dev = dev_id; > - struct netfront_info *np = netdev_priv(dev); > - unsigned long flags; > - > - spin_lock_irqsave(&np->tx_lock, flags); > - > - if (likely(netif_carrier_ok(dev))) { > - xennet_tx_buf_gc(dev); > - /* Under tx_lock: protects access to rx shared-ring indexes. */ > - if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > - napi_schedule(&np->napi); > - } > - > - spin_unlock_irqrestore(&np->tx_lock, flags); > - > - return IRQ_HANDLED; > -} > - > static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) > { > struct xen_netif_tx_sring *txs; > -- > 1.7.3
On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote:> On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote: > > add polling interface to xen-netfront device to support netconsole > > > > Ian, any thoughts on the spinlock changes?What are they for? At a guess they are a necessary consequence of the new calling context. However not all the drivers I looked at which supported netpool were using the irqsave variants in this context so I guess it must be some secondary effect. Anyway, the upshot is that I think the changelog needs to explain the rationale for the locking change. Ian.> > > Signed-off-by: Tina.Yang <tina.yang@oracle.com> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > > Signed-off-by: Zhenzhong.Duan <zhenzhong.duan@oracle.com> > > Tested-by: gurudas.pai <gurudas.pai@oracle.com> > > --- > > drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- > > 1 files changed, 34 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > > index fa67905..db638b4 100644 > > --- a/drivers/net/xen-netfront.c > > +++ b/drivers/net/xen-netfront.c > > @@ -489,6 +489,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > int frags = skb_shinfo(skb)->nr_frags; > > unsigned int offset = offset_in_page(data); > > unsigned int len = skb_headlen(skb); > > + unsigned long flags; > > > > frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); > > if (unlikely(frags > MAX_SKB_FRAGS + 1)) { > > @@ -498,12 +499,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > goto drop; > > } > > > > - spin_lock_irq(&np->tx_lock); > > + spin_lock_irqsave(&np->tx_lock, flags); > > > > if (unlikely(!netif_carrier_ok(dev) || > > (frags > 1 && !xennet_can_sg(dev)) || > > netif_needs_gso(skb, netif_skb_features(skb)))) { > > - spin_unlock_irq(&np->tx_lock); > > + spin_unlock_irqrestore(&np->tx_lock, flags); > > goto drop; > > } > > > > @@ -574,7 +575,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > > if (!netfront_tx_slot_available(np)) > > netif_stop_queue(dev); > > > > - spin_unlock_irq(&np->tx_lock); > > + spin_unlock_irqrestore(&np->tx_lock, flags); > > > > return NETDEV_TX_OK; > > > > @@ -1228,6 +1229,33 @@ static int xennet_set_features(struct net_device *dev, > > return 0; > > } > > > > +static irqreturn_t xennet_interrupt(int irq, void *dev_id) > > +{ > > + struct net_device *dev = dev_id; > > + struct netfront_info *np = netdev_priv(dev); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&np->tx_lock, flags); > > + > > + if (likely(netif_carrier_ok(dev))) { > > + xennet_tx_buf_gc(dev); > > + /* Under tx_lock: protects access to rx shared-ring indexes. */ > > + if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > > + napi_schedule(&np->napi); > > + } > > + > > + spin_unlock_irqrestore(&np->tx_lock, flags); > > + > > + return IRQ_HANDLED; > > +} > > + > > +#ifdef CONFIG_NET_POLL_CONTROLLER > > +static void xennet_poll_controller(struct net_device *dev) > > +{ > > + xennet_interrupt(0, dev); > > +} > > +#endif > > + > > static const struct net_device_ops xennet_netdev_ops = { > > .ndo_open = xennet_open, > > .ndo_uninit = xennet_uninit, > > @@ -1239,6 +1267,9 @@ static const struct net_device_ops xennet_netdev_ops = { > > .ndo_validate_addr = eth_validate_addr, > > .ndo_fix_features = xennet_fix_features, > > .ndo_set_features = xennet_set_features, > > +#ifdef CONFIG_NET_POLL_CONTROLLER > > + .ndo_poll_controller = xennet_poll_controller, > > +#endif > > }; > > > > static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) > > @@ -1448,26 +1479,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) > > return 0; > > } > > > > -static irqreturn_t xennet_interrupt(int irq, void *dev_id) > > -{ > > - struct net_device *dev = dev_id; > > - struct netfront_info *np = netdev_priv(dev); > > - unsigned long flags; > > - > > - spin_lock_irqsave(&np->tx_lock, flags); > > - > > - if (likely(netif_carrier_ok(dev))) { > > - xennet_tx_buf_gc(dev); > > - /* Under tx_lock: protects access to rx shared-ring indexes. */ > > - if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > > - napi_schedule(&np->napi); > > - } > > - > > - spin_unlock_irqrestore(&np->tx_lock, flags); > > - > > - return IRQ_HANDLED; > > -} > > - > > static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) > > { > > struct xen_netif_tx_sring *txs; > > -- > > 1.7.3
On 1/13/2012 3:06 AM, Ian Campbell wrote:> On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote: >> On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote: >>> add polling interface to xen-netfront device to support netconsole >>> >> Ian, any thoughts on the spinlock changes? > What are they for?When I did this patch back in 2008, both netconsole and netdump were supported. Spin_lock w/o irqsave and irqrestore would cause netdump hang due to the unexpected change of the irq status. Although netdump is now obsolete, I think it''s always a good practice to preserve caller''s irq status as we had a very bad experience chasing a similar problem caused by such a irq change in RDS in the not too long ago past.> At a guess they are a necessary consequence of the new calling context. > However not all the drivers I looked at which supported netpool were > using the irqsave variants in this context so I guess it must be some > secondary effect. > > Anyway, the upshot is that I think the changelog needs to explain the > rationale for the locking change. > > Ian. > >>> Signed-off-by: Tina.Yang<tina.yang@oracle.com> >>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com> >>> Cc: Jeremy Fitzhardinge<jeremy@goop.org> >>> Signed-off-by: Zhenzhong.Duan<zhenzhong.duan@oracle.com> >>> Tested-by: gurudas.pai<gurudas.pai@oracle.com> >>> --- >>> drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- >>> 1 files changed, 34 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >>> index fa67905..db638b4 100644 >>> --- a/drivers/net/xen-netfront.c >>> +++ b/drivers/net/xen-netfront.c >>> @@ -489,6 +489,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>> int frags = skb_shinfo(skb)->nr_frags; >>> unsigned int offset = offset_in_page(data); >>> unsigned int len = skb_headlen(skb); >>> + unsigned long flags; >>> >>> frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); >>> if (unlikely(frags> MAX_SKB_FRAGS + 1)) { >>> @@ -498,12 +499,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>> goto drop; >>> } >>> >>> - spin_lock_irq(&np->tx_lock); >>> + spin_lock_irqsave(&np->tx_lock, flags); >>> >>> if (unlikely(!netif_carrier_ok(dev) || >>> (frags> 1&& !xennet_can_sg(dev)) || >>> netif_needs_gso(skb, netif_skb_features(skb)))) { >>> - spin_unlock_irq(&np->tx_lock); >>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>> goto drop; >>> } >>> >>> @@ -574,7 +575,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>> if (!netfront_tx_slot_available(np)) >>> netif_stop_queue(dev); >>> >>> - spin_unlock_irq(&np->tx_lock); >>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>> >>> return NETDEV_TX_OK; >>> >>> @@ -1228,6 +1229,33 @@ static int xennet_set_features(struct net_device *dev, >>> return 0; >>> } >>> >>> +static irqreturn_t xennet_interrupt(int irq, void *dev_id) >>> +{ >>> + struct net_device *dev = dev_id; >>> + struct netfront_info *np = netdev_priv(dev); >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&np->tx_lock, flags); >>> + >>> + if (likely(netif_carrier_ok(dev))) { >>> + xennet_tx_buf_gc(dev); >>> + /* Under tx_lock: protects access to rx shared-ring indexes. */ >>> + if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) >>> + napi_schedule(&np->napi); >>> + } >>> + >>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +#ifdef CONFIG_NET_POLL_CONTROLLER >>> +static void xennet_poll_controller(struct net_device *dev) >>> +{ >>> + xennet_interrupt(0, dev); >>> +} >>> +#endif >>> + >>> static const struct net_device_ops xennet_netdev_ops = { >>> .ndo_open = xennet_open, >>> .ndo_uninit = xennet_uninit, >>> @@ -1239,6 +1267,9 @@ static const struct net_device_ops xennet_netdev_ops = { >>> .ndo_validate_addr = eth_validate_addr, >>> .ndo_fix_features = xennet_fix_features, >>> .ndo_set_features = xennet_set_features, >>> +#ifdef CONFIG_NET_POLL_CONTROLLER >>> + .ndo_poll_controller = xennet_poll_controller, >>> +#endif >>> }; >>> >>> static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) >>> @@ -1448,26 +1479,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) >>> return 0; >>> } >>> >>> -static irqreturn_t xennet_interrupt(int irq, void *dev_id) >>> -{ >>> - struct net_device *dev = dev_id; >>> - struct netfront_info *np = netdev_priv(dev); >>> - unsigned long flags; >>> - >>> - spin_lock_irqsave(&np->tx_lock, flags); >>> - >>> - if (likely(netif_carrier_ok(dev))) { >>> - xennet_tx_buf_gc(dev); >>> - /* Under tx_lock: protects access to rx shared-ring indexes. */ >>> - if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) >>> - napi_schedule(&np->napi); >>> - } >>> - >>> - spin_unlock_irqrestore(&np->tx_lock, flags); >>> - >>> - return IRQ_HANDLED; >>> -} >>> - >>> static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) >>> { >>> struct xen_netif_tx_sring *txs; >>> -- >>> 1.7.3 >
Konrad Rzeszutek Wilk
2012-Jan-17 21:51 UTC
Re: [PATCH] add netconsole support for xen-netfront
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:> On 1/13/2012 3:06 AM, Ian Campbell wrote: > >On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote: > >>On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote: > >>>add polling interface to xen-netfront device to support netconsole > >>> > >>Ian, any thoughts on the spinlock changes? > >What are they for? > When I did this patch back in 2008, both netconsole and netdump > were supported. Spin_lock w/o irqsave and irqrestore would cause > netdump hang due to the unexpected change of the irq status.Hm, that might have been due to the bug that was lurking in there since 2.6.27: d198d499148a0c64a41b3aba9e7dd43772832b91 "xen: x86_32: do not enable iterrupts when returning from exception in interrupt context"> Although netdump is now obsolete, I think it''s always a good practice > to preserve caller''s irq status as we had a very bad experience > chasing a similar problem caused by such a irq change in RDSDid you find the culprit of it? Was there a patch for that in the upstream kernel?> in the not too long ago past.OK, it sounds like it was issues in the past but might not be the case anymore. Could please re-test it without that spinlock irqsave patch using the upstream kernel (or just UEK2 since it is an 3.0 type kernel). Thanks.> >At a guess they are a necessary consequence of the new calling context. > >However not all the drivers I looked at which supported netpool were > >using the irqsave variants in this context so I guess it must be some > >secondary effect. > > > >Anyway, the upshot is that I think the changelog needs to explain the > >rationale for the locking change. > > > >Ian. > > > >>>Signed-off-by: Tina.Yang<tina.yang@oracle.com> > >>>Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com> > >>>Cc: Jeremy Fitzhardinge<jeremy@goop.org> > >>>Signed-off-by: Zhenzhong.Duan<zhenzhong.duan@oracle.com> > >>>Tested-by: gurudas.pai<gurudas.pai@oracle.com> > >>>--- > >>> drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- > >>> 1 files changed, 34 insertions(+), 23 deletions(-) > >>> > >>>diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > >>>index fa67905..db638b4 100644 > >>>--- a/drivers/net/xen-netfront.c > >>>+++ b/drivers/net/xen-netfront.c > >>>@@ -489,6 +489,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > >>> int frags = skb_shinfo(skb)->nr_frags; > >>> unsigned int offset = offset_in_page(data); > >>> unsigned int len = skb_headlen(skb); > >>>+ unsigned long flags; > >>> > >>> frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); > >>> if (unlikely(frags> MAX_SKB_FRAGS + 1)) { > >>>@@ -498,12 +499,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > >>> goto drop; > >>> } > >>> > >>>- spin_lock_irq(&np->tx_lock); > >>>+ spin_lock_irqsave(&np->tx_lock, flags); > >>> > >>> if (unlikely(!netif_carrier_ok(dev) || > >>> (frags> 1&& !xennet_can_sg(dev)) || > >>> netif_needs_gso(skb, netif_skb_features(skb)))) { > >>>- spin_unlock_irq(&np->tx_lock); > >>>+ spin_unlock_irqrestore(&np->tx_lock, flags); > >>> goto drop; > >>> } > >>> > >>>@@ -574,7 +575,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) > >>> if (!netfront_tx_slot_available(np)) > >>> netif_stop_queue(dev); > >>> > >>>- spin_unlock_irq(&np->tx_lock); > >>>+ spin_unlock_irqrestore(&np->tx_lock, flags); > >>> > >>> return NETDEV_TX_OK; > >>> > >>>@@ -1228,6 +1229,33 @@ static int xennet_set_features(struct net_device *dev, > >>> return 0; > >>> } > >>> > >>>+static irqreturn_t xennet_interrupt(int irq, void *dev_id) > >>>+{ > >>>+ struct net_device *dev = dev_id; > >>>+ struct netfront_info *np = netdev_priv(dev); > >>>+ unsigned long flags; > >>>+ > >>>+ spin_lock_irqsave(&np->tx_lock, flags); > >>>+ > >>>+ if (likely(netif_carrier_ok(dev))) { > >>>+ xennet_tx_buf_gc(dev); > >>>+ /* Under tx_lock: protects access to rx shared-ring indexes. */ > >>>+ if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > >>>+ napi_schedule(&np->napi); > >>>+ } > >>>+ > >>>+ spin_unlock_irqrestore(&np->tx_lock, flags); > >>>+ > >>>+ return IRQ_HANDLED; > >>>+} > >>>+ > >>>+#ifdef CONFIG_NET_POLL_CONTROLLER > >>>+static void xennet_poll_controller(struct net_device *dev) > >>>+{ > >>>+ xennet_interrupt(0, dev); > >>>+} > >>>+#endif > >>>+ > >>> static const struct net_device_ops xennet_netdev_ops = { > >>> .ndo_open = xennet_open, > >>> .ndo_uninit = xennet_uninit, > >>>@@ -1239,6 +1267,9 @@ static const struct net_device_ops xennet_netdev_ops = { > >>> .ndo_validate_addr = eth_validate_addr, > >>> .ndo_fix_features = xennet_fix_features, > >>> .ndo_set_features = xennet_set_features, > >>>+#ifdef CONFIG_NET_POLL_CONTROLLER > >>>+ .ndo_poll_controller = xennet_poll_controller, > >>>+#endif > >>> }; > >>> > >>> static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) > >>>@@ -1448,26 +1479,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) > >>> return 0; > >>> } > >>> > >>>-static irqreturn_t xennet_interrupt(int irq, void *dev_id) > >>>-{ > >>>- struct net_device *dev = dev_id; > >>>- struct netfront_info *np = netdev_priv(dev); > >>>- unsigned long flags; > >>>- > >>>- spin_lock_irqsave(&np->tx_lock, flags); > >>>- > >>>- if (likely(netif_carrier_ok(dev))) { > >>>- xennet_tx_buf_gc(dev); > >>>- /* Under tx_lock: protects access to rx shared-ring indexes. */ > >>>- if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) > >>>- napi_schedule(&np->napi); > >>>- } > >>>- > >>>- spin_unlock_irqrestore(&np->tx_lock, flags); > >>>- > >>>- return IRQ_HANDLED; > >>>-} > >>>- > >>> static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) > >>> { > >>> struct xen_netif_tx_sring *txs; > >>>-- > >>>1.7.3 > >
On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote:> On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote: >> On 1/13/2012 3:06 AM, Ian Campbell wrote: >>> On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote: >>>> On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote: >>>>> add polling interface to xen-netfront device to support netconsole >>>>> >>>> Ian, any thoughts on the spinlock changes? >>> What are they for? >> When I did this patch back in 2008, both netconsole and netdump >> were supported. Spin_lock w/o irqsave and irqrestore would cause >> netdump hang due to the unexpected change of the irq status. > Hm, that might have been due to the bug that was lurking in there since > 2.6.27: d198d499148a0c64a41b3aba9e7dd43772832b91 "xen: x86_32: do not enable iterrupts when returning from exception in interrupt context"Don''t think so. Btw, the (guest) kernels back then were el4 (2.6.9) and el5 (2.6.18).>> Although netdump is now obsolete, I think it''s always a good practice >> to preserve caller''s irq status as we had a very bad experience >> chasing a similar problem caused by such a irq change in RDS > Did you find the culprit of it? Was there a patch for that in the > upstream kernel?Yes. It has nothing to do with net drivers but same cause elsewhere in the kernel.>> in the not too long ago past. > OK, it sounds like it was issues in the past but might not be the > case anymore. > > Could please re-test it without that spinlock irqsave patch using > the upstream kernel (or just UEK2 since it is an 3.0 type kernel).Shouldn''t be the case now, but don''t know about the future. The fact is as long as there is a new caller that has the expectation of preserved irq status, it would be a problem. As Ian said, some net drivers have been cautious in this regard already by saving/restoring the status, but apparently not everyone.> Thanks. > >>> At a guess they are a necessary consequence of the new calling context. >>> However not all the drivers I looked at which supported netpool were >>> using the irqsave variants in this context so I guess it must be some >>> secondary effect. >>> >>> Anyway, the upshot is that I think the changelog needs to explain the >>> rationale for the locking change. >>> >>> Ian. >>> >>>>> Signed-off-by: Tina.Yang<tina.yang@oracle.com> >>>>> Cc: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com> >>>>> Cc: Jeremy Fitzhardinge<jeremy@goop.org> >>>>> Signed-off-by: Zhenzhong.Duan<zhenzhong.duan@oracle.com> >>>>> Tested-by: gurudas.pai<gurudas.pai@oracle.com> >>>>> --- >>>>> drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- >>>>> 1 files changed, 34 insertions(+), 23 deletions(-) >>>>> >>>>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >>>>> index fa67905..db638b4 100644 >>>>> --- a/drivers/net/xen-netfront.c >>>>> +++ b/drivers/net/xen-netfront.c >>>>> @@ -489,6 +489,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>>>> int frags = skb_shinfo(skb)->nr_frags; >>>>> unsigned int offset = offset_in_page(data); >>>>> unsigned int len = skb_headlen(skb); >>>>> + unsigned long flags; >>>>> >>>>> frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); >>>>> if (unlikely(frags> MAX_SKB_FRAGS + 1)) { >>>>> @@ -498,12 +499,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>>>> goto drop; >>>>> } >>>>> >>>>> - spin_lock_irq(&np->tx_lock); >>>>> + spin_lock_irqsave(&np->tx_lock, flags); >>>>> >>>>> if (unlikely(!netif_carrier_ok(dev) || >>>>> (frags> 1&& !xennet_can_sg(dev)) || >>>>> netif_needs_gso(skb, netif_skb_features(skb)))) { >>>>> - spin_unlock_irq(&np->tx_lock); >>>>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>>>> goto drop; >>>>> } >>>>> >>>>> @@ -574,7 +575,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) >>>>> if (!netfront_tx_slot_available(np)) >>>>> netif_stop_queue(dev); >>>>> >>>>> - spin_unlock_irq(&np->tx_lock); >>>>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>>>> >>>>> return NETDEV_TX_OK; >>>>> >>>>> @@ -1228,6 +1229,33 @@ static int xennet_set_features(struct net_device *dev, >>>>> return 0; >>>>> } >>>>> >>>>> +static irqreturn_t xennet_interrupt(int irq, void *dev_id) >>>>> +{ >>>>> + struct net_device *dev = dev_id; >>>>> + struct netfront_info *np = netdev_priv(dev); >>>>> + unsigned long flags; >>>>> + >>>>> + spin_lock_irqsave(&np->tx_lock, flags); >>>>> + >>>>> + if (likely(netif_carrier_ok(dev))) { >>>>> + xennet_tx_buf_gc(dev); >>>>> + /* Under tx_lock: protects access to rx shared-ring indexes. */ >>>>> + if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) >>>>> + napi_schedule(&np->napi); >>>>> + } >>>>> + >>>>> + spin_unlock_irqrestore(&np->tx_lock, flags); >>>>> + >>>>> + return IRQ_HANDLED; >>>>> +} >>>>> + >>>>> +#ifdef CONFIG_NET_POLL_CONTROLLER >>>>> +static void xennet_poll_controller(struct net_device *dev) >>>>> +{ >>>>> + xennet_interrupt(0, dev); >>>>> +} >>>>> +#endif >>>>> + >>>>> static const struct net_device_ops xennet_netdev_ops = { >>>>> .ndo_open = xennet_open, >>>>> .ndo_uninit = xennet_uninit, >>>>> @@ -1239,6 +1267,9 @@ static const struct net_device_ops xennet_netdev_ops = { >>>>> .ndo_validate_addr = eth_validate_addr, >>>>> .ndo_fix_features = xennet_fix_features, >>>>> .ndo_set_features = xennet_set_features, >>>>> +#ifdef CONFIG_NET_POLL_CONTROLLER >>>>> + .ndo_poll_controller = xennet_poll_controller, >>>>> +#endif >>>>> }; >>>>> >>>>> static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) >>>>> @@ -1448,26 +1479,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) >>>>> return 0; >>>>> } >>>>> >>>>> -static irqreturn_t xennet_interrupt(int irq, void *dev_id) >>>>> -{ >>>>> - struct net_device *dev = dev_id; >>>>> - struct netfront_info *np = netdev_priv(dev); >>>>> - unsigned long flags; >>>>> - >>>>> - spin_lock_irqsave(&np->tx_lock, flags); >>>>> - >>>>> - if (likely(netif_carrier_ok(dev))) { >>>>> - xennet_tx_buf_gc(dev); >>>>> - /* Under tx_lock: protects access to rx shared-ring indexes. */ >>>>> - if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) >>>>> - napi_schedule(&np->napi); >>>>> - } >>>>> - >>>>> - spin_unlock_irqrestore(&np->tx_lock, flags); >>>>> - >>>>> - return IRQ_HANDLED; >>>>> -} >>>>> - >>>>> static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) >>>>> { >>>>> struct xen_netif_tx_sring *txs; >>>>> -- >>>>> 1.7.3
On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote:> On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote: > > On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote: > >> On 1/13/2012 3:06 AM, Ian Campbell wrote: > >> Although netdump is now obsolete, I think it''s always a good practice > >> to preserve caller''s irq status as we had a very bad experience > >> chasing a similar problem caused by such a irq change in RDS > > Did you find the culprit of it? Was there a patch for that in the > > upstream kernel? > Yes. It has nothing to do with net drivers but same cause > elsewhere in the kernel.I didn''t think start_xmit could be called with interrupts disabled or from interrupt context but perhaps I am wrong about that or perhaps netconsole changes things? Right, Documentation/networking/netdevices.txt states that start_xmit can be called with interrupts disabled by netconsole and therefore using the irqsave/restore locking in this function is, AFAICT, correct.> >> in the not too long ago past. > > OK, it sounds like it was issues in the past but might not be the > > case anymore. > > > > Could please re-test it without that spinlock irqsave patch using > > the upstream kernel (or just UEK2 since it is an 3.0 type kernel). > Shouldn''t be the case now, but don''t know about the future. > The fact is as long as there is a new caller that has the expectation > of preserved irq status, it would be a problem.The question is not so much what may or may not be a problem in the future but what the requirements of this function are, in particular those imposed by the network stack for the start_xmit function.> As Ian said, some net drivers have been cautious in this regard already by > saving/restoring the status, but apparently not everyone.I was talking about the interrupt/poll handler here since I hadn''t yet noticed that the locking change was also in start_xmit and not just the poll/interrupt paths (which was actually just code motion and not a locking change in any case). Ian.
On 1/18/2012 12:59 AM, Ian Campbell wrote:> On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote: >> On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote: >>> On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote: >>>> On 1/13/2012 3:06 AM, Ian Campbell wrote: >>>> Although netdump is now obsolete, I think it''s always a good practice >>>> to preserve caller''s irq status as we had a very bad experience >>>> chasing a similar problem caused by such a irq change in RDS >>> Did you find the culprit of it? Was there a patch for that in the >>> upstream kernel? >> Yes. It has nothing to do with net drivers but same cause >> elsewhere in the kernel. > I didn''t think start_xmit could be called with interrupts disabled or > from interrupt context but perhaps I am wrong about that or perhaps > netconsole changes things?Netdump does call it with interrupt disabled and hang because of it in 2.6.9 as I remember it. And you are right, netconsole has undergone changes from time to time, which also can change this specification.> > Right, Documentation/networking/netdevices.txt states that start_xmit > can be called with interrupts disabled by netconsole and therefore using > the irqsave/restore locking in this function is, AFAICT, correct. > >>>> in the not too long ago past. >>> OK, it sounds like it was issues in the past but might not be the >>> case anymore. >>> >>> Could please re-test it without that spinlock irqsave patch using >>> the upstream kernel (or just UEK2 since it is an 3.0 type kernel). >> Shouldn''t be the case now, but don''t know about the future. >> The fact is as long as there is a new caller that has the expectation >> of preserved irq status, it would be a problem. > The question is not so much what may or may not be a problem in the > future but what the requirements of this function are, in particular > those imposed by the network stack for the start_xmit function.Agreed. It''s not safe to assume it unless the API documentation states that it can not be called with interrupt disabled explicitly.>> As Ian said, some net drivers have been cautious in this regard already by >> saving/restoring the status, but apparently not everyone. > I was talking about the interrupt/poll handler here since I hadn''t yet > noticed that the locking change was also in start_xmit and not just the > poll/interrupt paths (which was actually just code motion and not a > locking change in any case).I did look at the start_xmit of most of the net drivers myself when I hit the netdump hang back in 2008, and some of them did save/restore already, but others didn''t.> Ian. > >
Konrad Rzeszutek Wilk
2012-Jan-23 18:24 UTC
Re: [PATCH] add netconsole support for xen-netfront
On Wed, Jan 18, 2012 at 01:06:47PM -0800, Tina Yang wrote:> On 1/18/2012 12:59 AM, Ian Campbell wrote: > >On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote: > >>On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote: > >>>On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote: > >>>>On 1/13/2012 3:06 AM, Ian Campbell wrote: > >>>>Although netdump is now obsolete, I think it''s always a good practice > >>>>to preserve caller''s irq status as we had a very bad experience > >>>>chasing a similar problem caused by such a irq change in RDS > >>>Did you find the culprit of it? Was there a patch for that in the > >>>upstream kernel? > >>Yes. It has nothing to do with net drivers but same cause > >>elsewhere in the kernel. > >I didn''t think start_xmit could be called with interrupts disabled or > >from interrupt context but perhaps I am wrong about that or perhaps > >netconsole changes things? > Netdump does call it with interrupt disabled and hang because of > it in 2.6.9 as I remember it. And you are right, netconsole has > undergone changes from time to time, which also can change > this specification. > > > >Right, Documentation/networking/netdevices.txt states that start_xmit > >can be called with interrupts disabled by netconsole and therefore using > >the irqsave/restore locking in this function is, AFAICT, correct.Ok, so let me update the git commit description to include this. I am listed as the maintainer but I would have thought that it should go through David? David, should it go through you or should I stick it in my tree? Here is the patch with a better git description. From 1c265b7946f222ab6a5aac5245a0ab84618772c8 Mon Sep 17 00:00:00 2001 From: Zhenzhong Duan <zhenzhong.duan@oracle.com> Date: Thu, 12 Jan 2012 10:18:29 +0800 Subject: [PATCH] xen/netfront: add netconsole support. add polling interface to xen-netfront device to support netconsole This patch also alters the spin_lock usage to use irqsave variant. Documentation/networking/netdevices.txt states that start_xmit can be called with interrupts disabled by netconsole and therefore using the irqsave/restore locking in this function is looks correct. Signed-off-by: Tina.Yang <tina.yang@oracle.com> Cc: Jeremy Fitzhardinge <jeremy@goop.org> Signed-off-by: Zhenzhong.Duan <zhenzhong.duan@oracle.com> Tested-by: gurudas.pai <gurudas.pai@oracle.com> [v1: Copy-n-pasted Ian Campbell comments] Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++----------------- 1 files changed, 34 insertions(+), 23 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index d29365a..5a4b5df 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -478,6 +478,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) int frags = skb_shinfo(skb)->nr_frags; unsigned int offset = offset_in_page(data); unsigned int len = skb_headlen(skb); + unsigned long flags; frags += DIV_ROUND_UP(offset + len, PAGE_SIZE); if (unlikely(frags > MAX_SKB_FRAGS + 1)) { @@ -487,12 +488,12 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) goto drop; } - spin_lock_irq(&np->tx_lock); + spin_lock_irqsave(&np->tx_lock, flags); if (unlikely(!netif_carrier_ok(dev) || (frags > 1 && !xennet_can_sg(dev)) || netif_needs_gso(skb, netif_skb_features(skb)))) { - spin_unlock_irq(&np->tx_lock); + spin_unlock_irqrestore(&np->tx_lock, flags); goto drop; } @@ -561,7 +562,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev) if (!netfront_tx_slot_available(np)) netif_stop_queue(dev); - spin_unlock_irq(&np->tx_lock); + spin_unlock_irqrestore(&np->tx_lock, flags); return NETDEV_TX_OK; @@ -1176,6 +1177,33 @@ static int xennet_set_features(struct net_device *dev, u32 features) return 0; } +static irqreturn_t xennet_interrupt(int irq, void *dev_id) +{ + struct net_device *dev = dev_id; + struct netfront_info *np = netdev_priv(dev); + unsigned long flags; + + spin_lock_irqsave(&np->tx_lock, flags); + + if (likely(netif_carrier_ok(dev))) { + xennet_tx_buf_gc(dev); + /* Under tx_lock: protects access to rx shared-ring indexes. */ + if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) + napi_schedule(&np->napi); + } + + spin_unlock_irqrestore(&np->tx_lock, flags); + + return IRQ_HANDLED; +} + +#ifdef CONFIG_NET_POLL_CONTROLLER +static void xennet_poll_controller(struct net_device *dev) +{ + xennet_interrupt(0, dev); +} +#endif + static const struct net_device_ops xennet_netdev_ops = { .ndo_open = xennet_open, .ndo_uninit = xennet_uninit, @@ -1186,6 +1214,9 @@ static const struct net_device_ops xennet_netdev_ops = { .ndo_validate_addr = eth_validate_addr, .ndo_fix_features = xennet_fix_features, .ndo_set_features = xennet_set_features, +#ifdef CONFIG_NET_POLL_CONTROLLER + .ndo_poll_controller = xennet_poll_controller, +#endif }; static struct net_device * __devinit xennet_create_dev(struct xenbus_device *dev) @@ -1388,26 +1419,6 @@ static int xen_net_read_mac(struct xenbus_device *dev, u8 mac[]) return 0; } -static irqreturn_t xennet_interrupt(int irq, void *dev_id) -{ - struct net_device *dev = dev_id; - struct netfront_info *np = netdev_priv(dev); - unsigned long flags; - - spin_lock_irqsave(&np->tx_lock, flags); - - if (likely(netif_carrier_ok(dev))) { - xennet_tx_buf_gc(dev); - /* Under tx_lock: protects access to rx shared-ring indexes. */ - if (RING_HAS_UNCONSUMED_RESPONSES(&np->rx)) - napi_schedule(&np->napi); - } - - spin_unlock_irqrestore(&np->tx_lock, flags); - - return IRQ_HANDLED; -} - static int setup_netfront(struct xenbus_device *dev, struct netfront_info *info) { struct xen_netif_tx_sring *txs; -- 1.7.7.5
On Mon, 2012-01-23 at 18:24 +0000, Konrad Rzeszutek Wilk wrote:> Ok, so let me update the git commit description to include this. > > I am listed as the maintainer but I would have thought that it should > go through David?Generally folks listed as specific network driver maintainers are sub-maintainers under David, so the flow would be You->David->Linus. For netback I generally don''t bother with the "You->" bit but just Ack them on list and David picks them up.> David, should it go through you or should I stick it in my tree? Here > is the patch with a better git description. > > From 1c265b7946f222ab6a5aac5245a0ab84618772c8 Mon Sep 17 00:00:00 2001 > From: Zhenzhong Duan <zhenzhong.duan@oracle.com> > Date: Thu, 12 Jan 2012 10:18:29 +0800 > Subject: [PATCH] xen/netfront: add netconsole support. > > add polling interface to xen-netfront device to support netconsole > > This patch also alters the spin_lock usage to use irqsave variant. > Documentation/networking/netdevices.txt states that start_xmit > can be called with interrupts disabled by netconsole and therefore using > the irqsave/restore locking in this function is looks correct. > > Signed-off-by: Tina.Yang <tina.yang@oracle.com> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Signed-off-by: Zhenzhong.Duan <zhenzhong.duan@oracle.com> > Tested-by: gurudas.pai <gurudas.pai@oracle.com> > [v1: Copy-n-pasted Ian Campbell comments] > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>Acked-by: Ian Campbell <ian.campbell@citrix.com>
From: Ian Campbell <Ian.Campbell@citrix.com> Date: Tue, 24 Jan 2012 08:58:00 +0000> On Mon, 2012-01-23 at 18:24 +0000, Konrad Rzeszutek Wilk wrote: > >> From 1c265b7946f222ab6a5aac5245a0ab84618772c8 Mon Sep 17 00:00:00 2001 >> From: Zhenzhong Duan <zhenzhong.duan@oracle.com> >> Date: Thu, 12 Jan 2012 10:18:29 +0800 >> Subject: [PATCH] xen/netfront: add netconsole support. >> >> add polling interface to xen-netfront device to support netconsole >> >> This patch also alters the spin_lock usage to use irqsave variant. >> Documentation/networking/netdevices.txt states that start_xmit >> can be called with interrupts disabled by netconsole and therefore using >> the irqsave/restore locking in this function is looks correct. >> >> Signed-off-by: Tina.Yang <tina.yang@oracle.com> >> Cc: Jeremy Fitzhardinge <jeremy@goop.org> >> Signed-off-by: Zhenzhong.Duan <zhenzhong.duan@oracle.com> >> Tested-by: gurudas.pai <gurudas.pai@oracle.com> >> [v1: Copy-n-pasted Ian Campbell comments] >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>Applied.
Reasonably Related Threads
- [PATCH RESEND] net: convert xen-netfront to hw_features
- [PATCH RESEND] net: convert xen-netfront to hw_features
- [PATCH RESEND] net: convert xen-netfront to hw_features
- [PATCH 1/2] xen, libxc: init msix addr/data with value from qemu via hypercall
- [PATCH 2/3 v3] Refactor MSI restore call-chain to drop unnecessary argument