From: Ian Campbell <hidden> Date: 2012-01-13 11:06:08
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:
quoted
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.
From: Tina Yang <hidden> Date: 2012-01-17 21:42:41
On 1/13/2012 3:06 AM, Ian Campbell wrote:
On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote:
quoted
On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote:
quoted
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.
From: Konrad Rzeszutek Wilk <hidden> Date: 2012-01-17 21:53:11
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:
On 1/13/2012 3:06 AM, Ian Campbell wrote:
quoted
On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote:
quoted
On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote:
quoted
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.
quoted
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.
From: Tina Yang <hidden> Date: 2012-01-17 23:15:49
On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote:
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:
quoted
On 1/13/2012 3:06 AM, Ian Campbell wrote:
quoted
On Thu, 2012-01-12 at 14:17 +0000, Konrad Rzeszutek Wilk wrote:
quoted
On Wed, Jan 11, 2012 at 04:52:36PM +0800, Zhenzhong Duan wrote:
quoted
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).
quoted
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.
quoted
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.
quoted
quoted
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.
From: Ian Campbell <hidden> Date: 2012-01-18 08:59:40
On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote:
On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote:
quoted
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:
quoted
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.
quoted
quoted
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.
From: Tina Yang <hidden> Date: 2012-01-18 21:07:15
On 1/18/2012 12:59 AM, Ian Campbell wrote:
On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote:
quoted
On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote:
quoted
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:
quoted
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.
quoted
quoted
quoted
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.
quoted
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.
From: Konrad Rzeszutek Wilk <hidden> Date: 2012-01-23 18:27:13
On Wed, Jan 18, 2012 at 01:06:47PM -0800, Tina Yang wrote:
On 1/18/2012 12:59 AM, Ian Campbell wrote:
quoted
On Tue, 2012-01-17 at 23:15 +0000, Tina Yang wrote:
quoted
On 1/17/2012 1:51 PM, Konrad Rzeszutek Wilk wrote:
quoted
On Tue, Jan 17, 2012 at 01:42:22PM -0800, Tina Yang wrote:
quoted
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.
quoted
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 <redacted>
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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
Signed-off-by: Zhenzhong.Duan <redacted>
Tested-by: gurudas.pai <redacted>
[v1: Copy-n-pasted Ian Campbell comments]
Signed-off-by: Konrad Rzeszutek Wilk <redacted>
---
drivers/net/xen-netfront.c | 57 ++++++++++++++++++++++++++-----------------
1 files changed, 34 insertions(+), 23 deletions(-)
From: Ian Campbell <hidden> Date: 2012-01-24 08:58:08
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 <redacted>
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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
Signed-off-by: Zhenzhong.Duan <redacted>
Tested-by: gurudas.pai <redacted>
[v1: Copy-n-pasted Ian Campbell comments]
Signed-off-by: Konrad Rzeszutek Wilk <redacted>
From: David Miller <davem@davemloft.net> Date: 2012-01-25 04:02:56
From: Ian Campbell <redacted>
Date: Tue, 24 Jan 2012 08:58:00 +0000
On Mon, 2012-01-23 at 18:24 +0000, Konrad Rzeszutek Wilk wrote:
quoted
From 1c265b7946f222ab6a5aac5245a0ab84618772c8 Mon Sep 17 00:00:00 2001
From: Zhenzhong Duan <redacted>
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 <redacted>
Cc: Jeremy Fitzhardinge <redacted>
Signed-off-by: Zhenzhong.Duan <redacted>
Tested-by: gurudas.pai <redacted>
[v1: Copy-n-pasted Ian Campbell comments]
Signed-off-by: Konrad Rzeszutek Wilk <redacted>