[PATCH] hpsa: switch to pci_alloc_irq_vectors

[PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

Use pci_alloc_irq_vectors and drop the hand-crafted
interrupt affinity routines.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Cc: Don Brace <don.brace@microsemi.com>
---
 drivers/scsi/hpsa.c | 72 +++++++++++++++++++----------------------------------
 drivers/scsi/hpsa.h |  1 -
 2 files changed, 26 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4e82b69..104e699 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5618,7 +5618,7 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
 	sh->sg_tablesize = h->maxsgentries;
 	sh->transportt = hpsa_sas_transport_template;
 	sh->hostdata[0] = (unsigned long) h;
-	sh->irq = h->intr[h->intr_mode];
+	sh->irq = pci_irq_vector(h->pdev, h->intr_mode);
 	sh->unique_id = sh->irq;
 
 	h->scsi_host = sh;
@@ -7667,14 +7667,9 @@ static void hpsa_disable_interrupt_mode(struct ctlr_info *h)
  */
 static void hpsa_interrupt_mode(struct ctlr_info *h)
 {
+	unsigned int irq_flags = PCI_IRQ_LEGACY;
 #ifdef CONFIG_PCI_MSI
-	int err, i;
-	struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
-
-	for (i = 0; i < MAX_REPLY_QUEUES; i++) {
-		hpsa_msix_entries[i].vector = 0;
-		hpsa_msix_entries[i].entry = i;
-	}
+	int err;
 
 	/* Some boards advertise MSI but don't really support it */
 	if ((h->board_id == 0x40700E11) || (h->board_id == 0x40800E11) ||
@@ -7685,8 +7680,8 @@ static void hpsa_interrupt_mode(struct ctlr_info *h)
 		h->msix_vector = MAX_REPLY_QUEUES;
 		if (h->msix_vector > num_online_cpus())
 			h->msix_vector = num_online_cpus();
-		err = pci_enable_msix_range(h->pdev, hpsa_msix_entries,
-					    1, h->msix_vector);
+		err = pci_alloc_irq_vectors(h->pdev, 1, h->msix_vector,
+					    PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
 		if (err < 0) {
 			dev_warn(&h->pdev->dev, "MSI-X init failed %d\n", err);
 			h->msix_vector = 0;
@@ -7696,22 +7691,21 @@ static void hpsa_interrupt_mode(struct ctlr_info *h)
 			       "available\n", err);
 		}
 		h->msix_vector = err;
-		for (i = 0; i < h->msix_vector; i++)
-			h->intr[i] = hpsa_msix_entries[i].vector;
 		return;
 	}
 single_msi_mode:
 	if (pci_find_capability(h->pdev, PCI_CAP_ID_MSI)) {
 		dev_info(&h->pdev->dev, "MSI capable controller\n");
-		if (!pci_enable_msi(h->pdev))
+		if (!pci_enable_msi(h->pdev)) {
 			h->msi_vector = 1;
-		else
+			irq_flags = PCI_IRQ_MSI;
+		} else
 			dev_warn(&h->pdev->dev, "MSI init failed\n");
 	}
 default_int_mode:
 #endif				/* CONFIG_PCI_MSI */
 	/* if we get here we're going to use the default interrupt mode */
-	h->intr[h->intr_mode] = h->pdev->irq;
+	pci_alloc_irq_vectors(h->pdev, 1, 1, irq_flags);
 }
 
 static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
@@ -8236,17 +8230,6 @@ static int hpsa_alloc_cmd_pool(struct ctlr_info *h)
 	return -ENOMEM;
 }
 
-static void hpsa_irq_affinity_hints(struct ctlr_info *h)
-{
-	int i, cpu;
-
-	cpu = cpumask_first(cpu_online_mask);
-	for (i = 0; i < h->msix_vector; i++) {
-		irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
-		cpu = cpumask_next(cpu, cpu_online_mask);
-	}
-}
-
 /* clear affinity hints and free MSI-X, MSI, or legacy INTx vectors */
 static void hpsa_free_irqs(struct ctlr_info *h)
 {
@@ -8255,15 +8238,13 @@ static void hpsa_free_irqs(struct ctlr_info *h)
 	if (!h->msix_vector || h->intr_mode != PERF_MODE_INT) {
 		/* Single reply queue, only one irq to free */
 		i = h->intr_mode;
-		irq_set_affinity_hint(h->intr[i], NULL);
-		free_irq(h->intr[i], &h->q[i]);
+		free_irq(pci_irq_vector(h->pdev, i), &h->q[i]);
 		h->q[i] = 0;
 		return;
 	}
 
 	for (i = 0; i < h->msix_vector; i++) {
-		irq_set_affinity_hint(h->intr[i], NULL);
-		free_irq(h->intr[i], &h->q[i]);
+		free_irq(pci_irq_vector(h->pdev, i), &h->q[i]);
 		h->q[i] = 0;
 	}
 	for (; i < MAX_REPLY_QUEUES; i++)
@@ -8288,17 +8269,18 @@ static int hpsa_request_irqs(struct ctlr_info *h,
 		/* If performant mode and MSI-X, use multiple reply queues */
 		for (i = 0; i < h->msix_vector; i++) {
 			sprintf(h->intrname[i], "%s-msix%d", h->devname, i);
-			rc = request_irq(h->intr[i], msixhandler,
-					0, h->intrname[i],
-					&h->q[i]);
+			rc = request_irq(pci_irq_vector(h->pdev, i),
+					 msixhandler, 0, h->intrname[i],
+					 &h->q[i]);
 			if (rc) {
 				int j;
 
 				dev_err(&h->pdev->dev,
 					"failed to get irq %d for %s\n",
-				       h->intr[i], h->devname);
+					pci_irq_vector(h->pdev, i), h->devname);
 				for (j = 0; j < i; j++) {
-					free_irq(h->intr[j], &h->q[j]);
+					free_irq(pci_irq_vector(h->pdev, i),
+						 &h->q[j]);
 					h->q[j] = 0;
 				}
 				for (; j < MAX_REPLY_QUEUES; j++)
@@ -8306,7 +8288,6 @@ static int hpsa_request_irqs(struct ctlr_info *h,
 				return rc;
 			}
 		}
-		hpsa_irq_affinity_hints(h);
 	} else {
 		/* Use single reply pool */
 		if (h->msix_vector > 0 || h->msi_vector) {
@@ -8316,23 +8297,22 @@ static int hpsa_request_irqs(struct ctlr_info *h,
 			else
 				sprintf(h->intrname[h->intr_mode],
 					"%s-msi", h->devname);
-			rc = request_irq(h->intr[h->intr_mode],
-				msixhandler, 0,
-				h->intrname[h->intr_mode],
-				&h->q[h->intr_mode]);
+			rc = request_irq(pci_irq_vector(h->pdev, h->intr_mode),
+					 msixhandler, 0,
+					 h->intrname[h->intr_mode],
+					 &h->q[h->intr_mode]);
 		} else {
 			sprintf(h->intrname[h->intr_mode],
 				"%s-intx", h->devname);
-			rc = request_irq(h->intr[h->intr_mode],
-				intxhandler, IRQF_SHARED,
-				h->intrname[h->intr_mode],
-				&h->q[h->intr_mode]);
+			rc = request_irq(pci_irq_vector(h->pdev, h->intr_mode),
+					 intxhandler, IRQF_SHARED,
+					 h->intrname[h->intr_mode],
+					 &h->q[h->intr_mode]);
 		}
-		irq_set_affinity_hint(h->intr[h->intr_mode], NULL);
 	}
 	if (rc) {
 		dev_err(&h->pdev->dev, "failed to get irq %d for %s\n",
-		       h->intr[h->intr_mode], h->devname);
+			pci_irq_vector(h->pdev, h->intr_mode), h->devname);
 		hpsa_free_irqs(h);
 		return -ENODEV;
 	}
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 82cdfad..a10a24c 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -175,7 +175,6 @@ struct ctlr_info {
 #	define DOORBELL_INT	1
 #	define SIMPLE_MODE_INT	2
 #	define MEMQ_MODE_INT	3
-	unsigned int intr[MAX_REPLY_QUEUES];
 	unsigned int msix_vector;
 	unsigned int msi_vector;
 	int intr_mode; /* either PERF_MODE_INT or SIMPLE_MODE_INT */
-- 
1.8.5.6

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Tue, Nov 08, 2016 at 08:12:25AM +0100, Hannes Reinecke wrote:
> Use pci_alloc_irq_vectors and drop the hand-crafted
> interrupt affinity routines.

There are a couple more things we can do here.  I actually have a patch
in my tree that goes a little further, I'll post it in a bit.

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/08/2016 03:58 PM, Christoph Hellwig wrote:
> On Tue, Nov 08, 2016 at 08:12:25AM +0100, Hannes Reinecke wrote:
>> Use pci_alloc_irq_vectors and drop the hand-crafted
>> interrupt affinity routines.
>
> There are a couple more things we can do here.  I actually have a patch
> in my tree that goes a little further, I'll post it in a bit.
>
Right. I'll wait for it.

If you also happen to have patches for megaraid and mpt3sas I'd be very 
much interested in looking into them; I'm currently working on 
converting them, too.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Tue, Nov 08, 2016 at 04:00:46PM +0100, Hannes Reinecke wrote:
> Right. I'll wait for it.
>
> If you also happen to have patches for megaraid and mpt3sas I'd be very 
> much interested in looking into them; I'm currently working on converting 
> them, too.

I've looked at the them but haven't started work, so feel free to go
ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.

I've also started playing with lpfc, something I'll need to send to
you and James for testing and feedback soon.

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/08/2016 04:01 PM, Christoph Hellwig wrote:
> On Tue, Nov 08, 2016 at 04:00:46PM +0100, Hannes Reinecke wrote:
>> Right. I'll wait for it.
>>
>> If you also happen to have patches for megaraid and mpt3sas I'd be very
>> much interested in looking into them; I'm currently working on converting
>> them, too.
>
> I've looked at the them but haven't started work, so feel free to go
> ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.
>
> I've also started playing with lpfc, something I'll need to send to
> you and James for testing and feedback soon.
>
Bah. Another duplicate then.
Let's see who is faster :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Tue, Nov 08, 2016 at 04:02:54PM +0100, Hannes Reinecke wrote:
>> I've also started playing with lpfc, something I'll need to send to
>> you and James for testing and feedback soon.
>>
> Bah. Another duplicate then.
> Let's see who is faster :-)

lpfc depends on the series adding the post_vetors support, so I can't
send it until we have that merged.

RE: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Don Brace]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Tuesday, November 08, 2016 1:12 AM
> To: Martin K. Petersen
> Cc: Christoph Hellwig; James Bottomley; linux-scsi@vger.kernel.org; Hannes
> Reinecke; Hannes Reinecke; Don Brace
> Subject: [PATCH] hpsa: switch to pci_alloc_irq_vectors
> 
> EXTERNAL EMAIL
> 
> 
> Use pci_alloc_irq_vectors and drop the hand-crafted
> interrupt affinity routines.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> Cc: Don Brace <don.brace@microsemi.com>

Do we need to add an entry for map_queues?

Are you and Christoph still working on this patch? :)

Thanks,
Don Brace

ESC - Smart Storage
Microsemi Corporation




> ---
>  drivers/scsi/hpsa.c | 72 +++++++++++++++++++----------------------------------
>  drivers/scsi/hpsa.h |  1 -
>  2 files changed, 26 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 4e82b69..104e699 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -5618,7 +5618,7 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
>         sh->sg_tablesize = h->maxsgentries;
>         sh->transportt = hpsa_sas_transport_template;
>         sh->hostdata[0] = (unsigned long) h;
> -       sh->irq = h->intr[h->intr_mode];
> +       sh->irq = pci_irq_vector(h->pdev, h->intr_mode);
>         sh->unique_id = sh->irq;
> 
>         h->scsi_host = sh;
> @@ -7667,14 +7667,9 @@ static void hpsa_disable_interrupt_mode(struct
> ctlr_info *h)
>   */
>  static void hpsa_interrupt_mode(struct ctlr_info *h)
>  {
> +       unsigned int irq_flags = PCI_IRQ_LEGACY;
>  #ifdef CONFIG_PCI_MSI
> -       int err, i;
> -       struct msix_entry hpsa_msix_entries[MAX_REPLY_QUEUES];
> -
> -       for (i = 0; i < MAX_REPLY_QUEUES; i++) {
> -               hpsa_msix_entries[i].vector = 0;
> -               hpsa_msix_entries[i].entry = i;
> -       }
> +       int err;
> 
>         /* Some boards advertise MSI but don't really support it */
>         if ((h->board_id == 0x40700E11) || (h->board_id == 0x40800E11) ||
> @@ -7685,8 +7680,8 @@ static void hpsa_interrupt_mode(struct ctlr_info
> *h)
>                 h->msix_vector = MAX_REPLY_QUEUES;
>                 if (h->msix_vector > num_online_cpus())
>                         h->msix_vector = num_online_cpus();
> -               err = pci_enable_msix_range(h->pdev, hpsa_msix_entries,
> -                                           1, h->msix_vector);
> +               err = pci_alloc_irq_vectors(h->pdev, 1, h->msix_vector,
> +                                           PCI_IRQ_MSIX | PCI_IRQ_AFFINITY);
>                 if (err < 0) {
>                         dev_warn(&h->pdev->dev, "MSI-X init failed %d\n", err);
>                         h->msix_vector = 0;
> @@ -7696,22 +7691,21 @@ static void hpsa_interrupt_mode(struct ctlr_info
> *h)
>                                "available\n", err);
>                 }
>                 h->msix_vector = err;
> -               for (i = 0; i < h->msix_vector; i++)
> -                       h->intr[i] = hpsa_msix_entries[i].vector;
>                 return;
>         }
>  single_msi_mode:
>         if (pci_find_capability(h->pdev, PCI_CAP_ID_MSI)) {
>                 dev_info(&h->pdev->dev, "MSI capable controller\n");
> -               if (!pci_enable_msi(h->pdev))
> +               if (!pci_enable_msi(h->pdev)) {
>                         h->msi_vector = 1;
> -               else
> +                       irq_flags = PCI_IRQ_MSI;
> +               } else
>                         dev_warn(&h->pdev->dev, "MSI init failed\n");
>         }
>  default_int_mode:
>  #endif                         /* CONFIG_PCI_MSI */
>         /* if we get here we're going to use the default interrupt mode */
> -       h->intr[h->intr_mode] = h->pdev->irq;
> +       pci_alloc_irq_vectors(h->pdev, 1, 1, irq_flags);
>  }
> 
>  static int hpsa_lookup_board_id(struct pci_dev *pdev, u32 *board_id)
> @@ -8236,17 +8230,6 @@ static int hpsa_alloc_cmd_pool(struct ctlr_info *h)
>         return -ENOMEM;
>  }
> 
> -static void hpsa_irq_affinity_hints(struct ctlr_info *h)
> -{
> -       int i, cpu;
> -
> -       cpu = cpumask_first(cpu_online_mask);
> -       for (i = 0; i < h->msix_vector; i++) {
> -               irq_set_affinity_hint(h->intr[i], get_cpu_mask(cpu));
> -               cpu = cpumask_next(cpu, cpu_online_mask);
> -       }
> -}
> -
>  /* clear affinity hints and free MSI-X, MSI, or legacy INTx vectors */
>  static void hpsa_free_irqs(struct ctlr_info *h)
>  {
> @@ -8255,15 +8238,13 @@ static void hpsa_free_irqs(struct ctlr_info *h)
>         if (!h->msix_vector || h->intr_mode != PERF_MODE_INT) {
>                 /* Single reply queue, only one irq to free */
>                 i = h->intr_mode;
> -               irq_set_affinity_hint(h->intr[i], NULL);
> -               free_irq(h->intr[i], &h->q[i]);
> +               free_irq(pci_irq_vector(h->pdev, i), &h->q[i]);
>                 h->q[i] = 0;
>                 return;
>         }
> 
>         for (i = 0; i < h->msix_vector; i++) {
> -               irq_set_affinity_hint(h->intr[i], NULL);
> -               free_irq(h->intr[i], &h->q[i]);
> +               free_irq(pci_irq_vector(h->pdev, i), &h->q[i]);
>                 h->q[i] = 0;
>         }
>         for (; i < MAX_REPLY_QUEUES; i++)
> @@ -8288,17 +8269,18 @@ static int hpsa_request_irqs(struct ctlr_info *h,
>                 /* If performant mode and MSI-X, use multiple reply queues */
>                 for (i = 0; i < h->msix_vector; i++) {
>                         sprintf(h->intrname[i], "%s-msix%d", h->devname, i);
> -                       rc = request_irq(h->intr[i], msixhandler,
> -                                       0, h->intrname[i],
> -                                       &h->q[i]);
> +                       rc = request_irq(pci_irq_vector(h->pdev, i),
> +                                        msixhandler, 0, h->intrname[i],
> +                                        &h->q[i]);
>                         if (rc) {
>                                 int j;
> 
>                                 dev_err(&h->pdev->dev,
>                                         "failed to get irq %d for %s\n",
> -                                      h->intr[i], h->devname);
> +                                       pci_irq_vector(h->pdev, i), h->devname);
>                                 for (j = 0; j < i; j++) {
> -                                       free_irq(h->intr[j], &h->q[j]);
> +                                       free_irq(pci_irq_vector(h->pdev, i),
> +                                                &h->q[j]);
>                                         h->q[j] = 0;
>                                 }
>                                 for (; j < MAX_REPLY_QUEUES; j++)
> @@ -8306,7 +8288,6 @@ static int hpsa_request_irqs(struct ctlr_info *h,
>                                 return rc;
>                         }
>                 }
> -               hpsa_irq_affinity_hints(h);
>         } else {
>                 /* Use single reply pool */
>                 if (h->msix_vector > 0 || h->msi_vector) {
> @@ -8316,23 +8297,22 @@ static int hpsa_request_irqs(struct ctlr_info *h,
>                         else
>                                 sprintf(h->intrname[h->intr_mode],
>                                         "%s-msi", h->devname);
> -                       rc = request_irq(h->intr[h->intr_mode],
> -                               msixhandler, 0,
> -                               h->intrname[h->intr_mode],
> -                               &h->q[h->intr_mode]);
> +                       rc = request_irq(pci_irq_vector(h->pdev, h->intr_mode),
> +                                        msixhandler, 0,
> +                                        h->intrname[h->intr_mode],
> +                                        &h->q[h->intr_mode]);
>                 } else {
>                         sprintf(h->intrname[h->intr_mode],
>                                 "%s-intx", h->devname);
> -                       rc = request_irq(h->intr[h->intr_mode],
> -                               intxhandler, IRQF_SHARED,
> -                               h->intrname[h->intr_mode],
> -                               &h->q[h->intr_mode]);
> +                       rc = request_irq(pci_irq_vector(h->pdev, h->intr_mode),
> +                                        intxhandler, IRQF_SHARED,
> +                                        h->intrname[h->intr_mode],
> +                                        &h->q[h->intr_mode]);
>                 }
> -               irq_set_affinity_hint(h->intr[h->intr_mode], NULL);
>         }
>         if (rc) {
>                 dev_err(&h->pdev->dev, "failed to get irq %d for %s\n",
> -                      h->intr[h->intr_mode], h->devname);
> +                       pci_irq_vector(h->pdev, h->intr_mode), h->devname);
>                 hpsa_free_irqs(h);
>                 return -ENODEV;
>         }
> diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
> index 82cdfad..a10a24c 100644
> --- a/drivers/scsi/hpsa.h
> +++ b/drivers/scsi/hpsa.h
> @@ -175,7 +175,6 @@ struct ctlr_info {
>  #      define DOORBELL_INT     1
>  #      define SIMPLE_MODE_INT  2
>  #      define MEMQ_MODE_INT    3
> -       unsigned int intr[MAX_REPLY_QUEUES];
>         unsigned int msix_vector;
>         unsigned int msi_vector;
>         int intr_mode; /* either PERF_MODE_INT or SIMPLE_MODE_INT */
> --
> 1.8.5.6

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Wed, Nov 09, 2016 at 03:32:48PM +0000, Don Brace wrote:
> Do we need to add an entry for map_queues?

The existing driver only supports a single submission queue.
If some of the devices support more than one submission queue
we could enhance the driver to support it, but we'd need docs
and hardware.

> Are you and Christoph still working on this patch? :)

I just need to post the version I have here after rebasing it.
Shouldn't take long, but I'm fighting a few fires at the moment,
hopefully later today.

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/09/2016 04:36 PM, Christoph Hellwig wrote:
> On Wed, Nov 09, 2016 at 03:32:48PM +0000, Don Brace wrote:
>> Do we need to add an entry for map_queues?
>
> The existing driver only supports a single submission queue.
> If some of the devices support more than one submission queue
> we could enhance the driver to support it, but we'd need docs
> and hardware.
>
Hardware is not an issue :-)

However, I'm still curious about the layout of the queues.
Are all queues (in performant mode) identical and can we use them for 
parallel submission? Or do we have to treat them differently?
(There were some rumours that it actually uses the queues for different 
I/O sizes ...)

Once that is cleared up it shouldn't be too hard moving to full 
multiqueue support.
(If we have identical queues, that is :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Don Brace]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Wednesday, November 09, 2016 9:45 AM
> To: Christoph Hellwig; Don Brace
> Cc: Martin K. Petersen; James Bottomley; linux-scsi@vger.kernel.org; Hannes
> Reinecke
> Subject: Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors
> 
> EXTERNAL EMAIL
> 
> 
> On 11/09/2016 04:36 PM, Christoph Hellwig wrote:
> > On Wed, Nov 09, 2016 at 03:32:48PM +0000, Don Brace wrote:
> >> Do we need to add an entry for map_queues?
> >
> > The existing driver only supports a single submission queue.
> > If some of the devices support more than one submission queue
> > we could enhance the driver to support it, but we'd need docs
> > and hardware.
> >
> Hardware is not an issue :-)
> 
> However, I'm still curious about the layout of the queues.
> Are all queues (in performant mode) identical and can we use them for
> parallel submission? Or do we have to treat them differently?\

All of the submission queues are identical.

> (There were some rumours that it actually uses the queues for different
> I/O sizes ...)

This is an untrue rumor. :)

Thanks,
Don Brace

ESC - Smart Storage
Microsemi Corporation



> 
> Once that is cleared up it shouldn't be too hard moving to full
> multiqueue support.
> (If we have identical queues, that is :-)
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke                Teamlead Storage & Networking
> hare@suse.de                                   +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Tue, Nov 08, 2016 at 04:02:54PM +0100, Hannes Reinecke wrote:
>> I've looked at the them but haven't started work, so feel free to go
>> ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.
>>
>> I've also started playing with lpfc, something I'll need to send to
>> you and James for testing and feedback soon.
>>
> Bah. Another duplicate then.
> Let's see who is faster :-)

Here are the lpfc patches - note that it's in a branch that merges
the recent irq affinity changes from the tip tree into the scsi tree
first, as we need updates from both of them:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/lpfc-msix

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/09/2016 10:39 PM, Christoph Hellwig wrote:
> On Tue, Nov 08, 2016 at 04:02:54PM +0100, Hannes Reinecke wrote:
>>> I've looked at the them but haven't started work, so feel free to go
>>> ahead.  The MSI-X handling in mpt3sas is pretty nasty, so be careful.
>>>
>>> I've also started playing with lpfc, something I'll need to send to
>>> you and James for testing and feedback soon.
>>>
>> Bah. Another duplicate then.
>> Let's see who is faster :-)
> 
> Here are the lpfc patches - note that it's in a branch that merges
> the recent irq affinity changes from the tip tree into the scsi tree
> first, as we need updates from both of them:
> 
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/lpfc-msix
> 
What I find quite irritating is that we still have to call
irq_set_affinity_hint(irq, NULL) when freeing up interrupts.
Can't we roll that into the call to free_irq() ?
Thing is, we're not _calling_ irq_set_affinity_hint() when setting up
the interrupt, so it'll be one of the things which will be easily forgotten.

And you _do_ have to setup a cpumap within the driver :-)

Anyway, remainder looks pretty close to what I've written up.
Feel free to add my 'Reviewed-by:' to it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Christoph Hellwig]

On Thu, Nov 10, 2016 at 07:48:20AM +0100, Hannes Reinecke wrote:
> What I find quite irritating is that we still have to call
> irq_set_affinity_hint(irq, NULL) when freeing up interrupts.
> Can't we roll that into the call to free_irq() ?

If you do call it that's irritation, because you should not call
irq_set_affinity_hint ever if you are using PCI_IRQ_AFFINITY, and
the above branch doesn't call it.

> And you _do_ have to setup a cpumap within the driver :-)

For the non-mq case, yes - but only to keep the functionality
as-is.  We shouldn't add anything like this to a new driver.

> Anyway, remainder looks pretty close to what I've written up.
> Feel free to add my 'Reviewed-by:' to it.

I'm mostly looking for someone who has the hardware to actually
test it, though.

Re: [PATCH] hpsa: switch to pci_alloc_irq_vectors

[Hannes Reinecke]

On 11/10/2016 03:48 PM, Christoph Hellwig wrote:
> On Thu, Nov 10, 2016 at 07:48:20AM +0100, Hannes Reinecke wrote:
>> What I find quite irritating is that we still have to call
>> irq_set_affinity_hint(irq, NULL) when freeing up interrupts.
>> Can't we roll that into the call to free_irq() ?
> 
> If you do call it that's irritation, because you should not call
> irq_set_affinity_hint ever if you are using PCI_IRQ_AFFINITY, and
> the above branch doesn't call it.
> 
Ah. Sorry to have misread that.

>> And you _do_ have to setup a cpumap within the driver :-)
> 
> For the non-mq case, yes - but only to keep the functionality
> as-is.  We shouldn't add anything like this to a new driver.
> 
>> Anyway, remainder looks pretty close to what I've written up.
>> Feel free to add my 'Reviewed-by:' to it.
> 
> I'm mostly looking for someone who has the hardware to actually
> test it, though.
> 
Ah. Right.

Will give it a spin tomorrow.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)