Re: [PATCH] add netconsole support for xen-netfront

Re: [PATCH] add netconsole support for xen-netfront

[Konrad Rzeszutek Wilk]

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

Re: [PATCH] add netconsole support for xen-netfront

[Tina Yang]

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
>

Re: [PATCH] add netconsole support for xen-netfront

[Konrad Rzeszutek Wilk]

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 RDS

Did 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
> >

Re: [PATCH] add netconsole support for xen-netfront

[Tina Yang]

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

Re: [PATCH] add netconsole support for xen-netfront

[Ian Campbell]

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.

Re: [PATCH] add netconsole support for xen-netfront

[Tina Yang]

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.
>
>

Re: [PATCH] add netconsole support for xen-netfront

[Konrad Rzeszutek Wilk]

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

Re: [PATCH] add netconsole support for xen-netfront

[Ian Campbell]

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>

Re: [PATCH] add netconsole support for xen-netfront

[David Miller]

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.