2014-10-29 15:15:26

by Fabian Frédérick

[permalink] [raw]
Subject: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc

Fix -Wunused-but-set-variable warning

Signed-off-by: Fabian Frederick <[email protected]>
---
drivers/scsi/hpsa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index cef5d49..34330e1 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6619,11 +6619,11 @@ static void hpsa_free_cmd_pool(struct ctlr_info *h)

static void hpsa_irq_affinity_hints(struct ctlr_info *h)
{
- int i, cpu, rc;
+ int i, cpu;

cpu = cpumask_first(cpu_online_mask);
for (i = 0; i < h->msix_vector; i++) {
- rc = irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
+ irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
cpu = cpumask_next(cpu, cpu_online_mask);
}
}
--
1.9.3


2014-10-30 05:55:26

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc

On Wed, Oct 29, 2014 at 04:15:04PM +0100, Fabian Frederick wrote:
> Fix -Wunused-but-set-variable warning

you should also mention why you have left the call to irq_set_affinity_hint().
i am not sure , but it looks like irq_set_affinity_hint() is only checking if
the lock is available or not. It is just locking ,then if lock is successfull then
returning 0 or if lock fails then return -EINVAL, and unlocks before returnig.
not doing anything else.

thanks
sudip

>
> Signed-off-by: Fabian Frederick <[email protected]>
> ---
> drivers/scsi/hpsa.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index cef5d49..34330e1 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -6619,11 +6619,11 @@ static void hpsa_free_cmd_pool(struct ctlr_info *h)
>
> static void hpsa_irq_affinity_hints(struct ctlr_info *h)
> {
> - int i, cpu, rc;
> + int i, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> for (i = 0; i < h->msix_vector; i++) {
> - rc = irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> + irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> cpu = cpumask_next(cpu, cpu_online_mask);
> }
> }
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

Subject: RE: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc



> -----Original Message-----
> From: [email protected] [mailto:linux-scsi-
> [email protected]] On Behalf Of Sudip Mukherjee
> Sent: Thursday, October 30, 2014 12:55 AM
> To: Fabian Frederick
> Cc: [email protected]; Stephen M. Cameron; James E.J.
> Bottomley; [email protected]; [email protected]
> Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable
> rc
>
> On Wed, Oct 29, 2014 at 04:15:04PM +0100, Fabian Frederick wrote:
> > Fix -Wunused-but-set-variable warning
>
> you should also mention why you have left the call to
> irq_set_affinity_hint().
> i am not sure , but it looks like irq_set_affinity_hint() is only
> checking if
> the lock is available or not. It is just locking ,then if lock is
> successfull then
> returning 0 or if lock fails then return -EINVAL, and unlocks before
> returnig.
> not doing anything else.
>
> thanks
> sudip

No, that function sets a mask value that shows up in
/proc/irq/nnn/affinity_hint
that a program like irqbalance may use to set the CPU affinity mask
for each irq via
/proc/irq/nnn/smp_affinity (bitmap format)
/proc/irq/nnn/smp_affinity_list (range format)

The reason is that in many cases, it is best when all these occur
on the same CPU that submitted the IO:
* LLD submission queues (if multiple are supported)
* LDD completion queues
* MSI-X interrupt indicating completion
* LLD completion interrupt handler
* block layer completion handler

Benefits include:
* cache efficiency - the data structures for the IO aren't
pulled from CPU to CPU.

* avoid IPI overhead in the block layer to get the completion
processed on the submitting CPU (which is done if using
rq_affinity=2 and the interrupt is routed to on another CPU).

* self-throttle the CPUs - avoid swamping one CPU with
completion processing for IOs submitted by many other CPUs
(which leads to stalls on the victim and timeouts on the
aggressors).

You must run irqbalance with an option to honor the hints;
some versions default to that, others don't. Or, disable
the irqbalance service and set them up the affinities
manually.

2014-10-30 06:40:30

by Sudip Mukherjee

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc

On Thu, Oct 30, 2014 at 06:28:13AM +0000, Elliott, Robert (Server Storage) wrote:
>
>
> > -----Original Message-----
> > From: [email protected] [mailto:linux-scsi-
> > [email protected]] On Behalf Of Sudip Mukherjee
> > Sent: Thursday, October 30, 2014 12:55 AM
> > To: Fabian Frederick
> > Cc: [email protected]; Stephen M. Cameron; James E.J.
> > Bottomley; [email protected]; [email protected]
> > Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable
> > rc
> >
> > On Wed, Oct 29, 2014 at 04:15:04PM +0100, Fabian Frederick wrote:
> > > Fix -Wunused-but-set-variable warning
> >
> > you should also mention why you have left the call to
> > irq_set_affinity_hint().
> > i am not sure , but it looks like irq_set_affinity_hint() is only
> > checking if
> > the lock is available or not. It is just locking ,then if lock is
> > successfull then
> > returning 0 or if lock fails then return -EINVAL, and unlocks before
> > returnig.
> > not doing anything else.
> >
> > thanks
> > sudip
>
> No, that function sets a mask value that shows up in
> /proc/irq/nnn/affinity_hint
> that a program like irqbalance may use to set the CPU affinity mask
> for each irq via
> /proc/irq/nnn/smp_affinity (bitmap format)
> /proc/irq/nnn/smp_affinity_list (range format)
>
> The reason is that in many cases, it is best when all these occur
> on the same CPU that submitted the IO:
> * LLD submission queues (if multiple are supported)
> * LDD completion queues
> * MSI-X interrupt indicating completion
> * LLD completion interrupt handler
> * block layer completion handler
>
> Benefits include:
> * cache efficiency - the data structures for the IO aren't
> pulled from CPU to CPU.
>
> * avoid IPI overhead in the block layer to get the completion
> processed on the submitting CPU (which is done if using
> rq_affinity=2 and the interrupt is routed to on another CPU).
>
> * self-throttle the CPUs - avoid swamping one CPU with
> completion processing for IOs submitted by many other CPUs
> (which leads to stalls on the victim and timeouts on the
> aggressors).
>
> You must run irqbalance with an option to honor the hints;
> some versions default to that, others don't. Or, disable
> the irqbalance service and set them up the affinities
> manually.
>
thanks , i think this is happening at desc->affinity_hint = m;

thanks
sudip
>

2014-10-30 15:21:10

by Don Brace

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc

On 10/29/2014 10:15 AM, Fabian Frederick wrote:
> Fix -Wunused-but-set-variable warning
>
> Signed-off-by: Fabian Frederick <[email protected]>
> ---
> drivers/scsi/hpsa.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index cef5d49..34330e1 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -6619,11 +6619,11 @@ static void hpsa_free_cmd_pool(struct ctlr_info *h)
>
> static void hpsa_irq_affinity_hints(struct ctlr_info *h)
> {
> - int i, cpu, rc;
> + int i, cpu;
>
> cpu = cpumask_first(cpu_online_mask);
> for (i = 0; i < h->msix_vector; i++) {
> - rc = irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> + irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> cpu = cpumask_next(cpu, cpu_online_mask);
> }
> }
>

I can add this patch to my current set of patches for 3.19 or ack this
now, What would you prefer?

---
Don Brace
[email protected]

2014-10-30 15:27:42

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/1 linux-next] hpsa: remove set but unused variable rc

On Thu, Oct 30, 2014 at 10:21:05AM -0500, Don Brace wrote:
> I can add this patch to my current set of patches for 3.19 or ack this now,
> What would you prefer?

Please queue it up, I'm much more interested in reducing the massive
backlog of real hpsa changes than in litte cleanups at the moment.