[PATCHv4 3/4] sparc64: Avoid irqsave/restore on vio.lock if in_softirq()

[Sowmini Varadhan]

For NAPIfied drivers , there is no need to
synchronize by doing irqsave/restore on vio.lock in the I/O
path.

Signed-off-by: Sowmini Varadhan <***@oracle.com>
---
arch/sparc/kernel/viohs.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c
index 7ef081a..d731586 100644
--- a/arch/sparc/kernel/viohs.c
+++ b/arch/sparc/kernel/viohs.c
@@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free);

void vio_port_up(struct vio_driver_state *vio)
{
- unsigned long flags;
+ unsigned long flags = 0;
int err, state;

- spin_lock_irqsave(&vio->lock, flags);
+ if (!in_softirq())
+ spin_lock_irqsave(&vio->lock, flags);

state = ldc_state(vio->lp);

@@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio)
mod_timer(&vio->timer, expires);
}

- spin_unlock_irqrestore(&vio->lock, flags);
+ if (!in_softirq())
+ spin_unlock_irqrestore(&vio->lock, flags);
}
EXPORT_SYMBOL(vio_port_up);

[Julian Calaby]

Hi Sowmini,

On Wed, Oct 22, 2014 at 1:16 AM, Sowmini Varadhan

Post by Sowmini Varadhan
For NAPIfied drivers , there is no need to
synchronize by doing irqsave/restore on vio.lock in the I/O
path.
---
arch/sparc/kernel/viohs.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c
index 7ef081a..d731586 100644
--- a/arch/sparc/kernel/viohs.c
+++ b/arch/sparc/kernel/viohs.c
@@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free);
void vio_port_up(struct vio_driver_state *vio)
{
- unsigned long flags;
+ unsigned long flags = 0;

Is gcc not smart enough to know that this variable isn't used before
it's set? (I assume it isn't used elsewhere in this function)

Post by Sowmini Varadhan
int err, state;
- spin_lock_irqsave(&vio->lock, flags);
+ if (!in_softirq())
+ spin_lock_irqsave(&vio->lock, flags);
state = ldc_state(vio->lp);
@@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio)
mod_timer(&vio->timer, expires);
}
- spin_unlock_irqrestore(&vio->lock, flags);
+ if (!in_softirq())
+ spin_unlock_irqrestore(&vio->lock, flags);
}
EXPORT_SYMBOL(vio_port_up);

Thanks,

[Sowmini Varadhan]

Post by Julian Calaby

Post by Sowmini Varadhan
void vio_port_up(struct vio_driver_state *vio)
{
- unsigned long flags;
+ unsigned long flags = 0;

Is gcc not smart enough to know that this variable isn't used before
it's set? (I assume it isn't used elsewhere in this function)

No, it's not used elsewhere in the function, and yes, I too was
surprised by the build warning, which is why I initialized it
as above.

--Sowmini

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Julian Calaby]

Hi Sowmini,

On Wed, Oct 22, 2014 at 9:39 AM, Sowmini Varadhan

Post by Sowmini Varadhan

Post by Julian Calaby

Post by Sowmini Varadhan
void vio_port_up(struct vio_driver_state *vio)
{
- unsigned long flags;
+ unsigned long flags = 0;

Is gcc not smart enough to know that this variable isn't used before
it's set? (I assume it isn't used elsewhere in this function)

No, it's not used elsewhere in the function, and yes, I too was
surprised by the build warning, which is why I initialized it
as above.

Ok, fair enough then.

Thanks,

[Dave Kleikamp]

Post by Julian Calaby
Hi Sowmini,
On Wed, Oct 22, 2014 at 1:16 AM, Sowmini Varadhan

Post by Sowmini Varadhan
For NAPIfied drivers , there is no need to
synchronize by doing irqsave/restore on vio.lock in the I/O
path.
---
arch/sparc/kernel/viohs.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/sparc/kernel/viohs.c b/arch/sparc/kernel/viohs.c
index 7ef081a..d731586 100644
--- a/arch/sparc/kernel/viohs.c
+++ b/arch/sparc/kernel/viohs.c
@@ -747,10 +747,11 @@ EXPORT_SYMBOL(vio_ldc_free);
void vio_port_up(struct vio_driver_state *vio)
{
- unsigned long flags;
+ unsigned long flags = 0;

Is gcc not smart enough to know that this variable isn't used before
it's set? (I assume it isn't used elsewhere in this function)

It probably assumes in_softirq() might evaluate differently in the each
case.

Post by Julian Calaby

Post by Sowmini Varadhan
int err, state;
- spin_lock_irqsave(&vio->lock, flags);
+ if (!in_softirq())
+ spin_lock_irqsave(&vio->lock, flags);
state = ldc_state(vio->lp);
@@ -777,7 +778,8 @@ void vio_port_up(struct vio_driver_state *vio)
mod_timer(&vio->timer, expires);
}
- spin_unlock_irqrestore(&vio->lock, flags);
+ if (!in_softirq())
+ spin_unlock_irqrestore(&vio->lock, flags);
}
EXPORT_SYMBOL(vio_port_up);

Thanks,

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Sowmini Varadhan]

Post by Dave Kleikamp

Post by Julian Calaby
Is gcc not smart enough to know that this variable isn't used before
it's set? (I assume it isn't used elsewhere in this function)

It probably assumes in_softirq() might evaluate differently in the each
case.

yes, that's what I suspected too. I suppose it is possible
from the compiler's point of view that something in between
might change the result of in_softirq() so that we may be
using an uninit variable in the second call.

anyway, the warning was annoying, and would only numb the
user into ignoring other real issues, so I figured I might
as well silence the warning.

--Sowmini

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html