[PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Fri, 19 Jul 2019 15:03:06 +1000
Subject: 

Another issue with the Apple T2 based 2018 controllers seem to be
that they blow up (and shut the machine down) if there's a tag
collision between the IO queue and the Admin queue.

My suspicion is that they use our tags for their internal tracking
and don't mix them with the queue id. They also seem to not like
when tags go beyond the IO queue depth, ie 128 tags.

This adds a quirk that marks tags 0..31 of the IO queue reserved

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Thanks Damien, reserved tags work and make this a lot simpler !

 drivers/nvme/host/nvme.h |  5 +++++
 drivers/nvme/host/pci.c  | 19 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ced0e0a7e039..8732da6df555 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -102,6 +102,11 @@ enum nvme_quirks {
 	 * Use non-standard 128 bytes SQEs.
 	 */
 	NVME_QUIRK_128_BYTES_SQES		= (1 << 11),
+
+	/*
+	 * Prevent tag overlap between queues
+	 */
+	NVME_QUIRK_SHARED_TAGS                  = (1 << 12),
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 7088971d4c42..fc74395a028b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2106,6 +2106,14 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	unsigned long size;
 
 	nr_io_queues = max_io_queues();
+
+	/*
+	 * If tags are shared with admin queue (Apple bug), then
+	 * make sure we only use one IO queue.
+	 */
+	if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
+		nr_io_queues = 1;
+
 	result = nvme_set_queue_count(&dev->ctrl, &nr_io_queues);
 	if (result < 0)
 		return result;
@@ -2278,6 +2286,14 @@ static int nvme_dev_add(struct nvme_dev *dev)
 		dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
 		dev->tagset.driver_data = dev;
 
+		/*
+		 * Some Apple controllers requires tags to be unique
+		 * across admin and IO queue, so reserve the first 32
+		 * tags of the IO queue.
+		 */
+		if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
+			dev->tagset.reserved_tags = NVME_AQ_DEPTH;
+
 		ret = blk_mq_alloc_tag_set(&dev->tagset);
 		if (ret) {
 			dev_warn(dev->ctrl.device,
@@ -3057,7 +3073,8 @@ static const struct pci_device_id nvme_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
 	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
 		.driver_data = NVME_QUIRK_SINGLE_VECTOR |
-				NVME_QUIRK_128_BYTES_SQES },
+				NVME_QUIRK_128_BYTES_SQES |
+				NVME_QUIRK_SHARED_TAGS },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, nvme_id_table);

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Balbir Singh]

On Fri, Jul 19, 2019 at 3:31 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Fri, 19 Jul 2019 15:03:06 +1000
> Subject:
>
> Another issue with the Apple T2 based 2018 controllers seem to be
> that they blow up (and shut the machine down) if there's a tag
> collision between the IO queue and the Admin queue.
>
> My suspicion is that they use our tags for their internal tracking
> and don't mix them with the queue id. They also seem to not like
> when tags go beyond the IO queue depth, ie 128 tags.
>
> This adds a quirk that marks tags 0..31 of the IO queue reserved
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---

Reviewed-by: Balbir Singh <bsingharora@gmail.com>

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Christoph Hellwig]

Yikes, that things looks worse and worse.  I think at this point we'll
have to defer the support to 5.4 unfortunately as it is getting more
and more involved..

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Fri, 2019-07-19 at 14:28 +0200, Christoph Hellwig wrote:
> Yikes, that things looks worse and worse.  I think at this point
> we'll
> have to defer the support to 5.4 unfortunately as it is getting more
> and more involved..

Well, at least v3 of that patch, thanks to Damien's idea, isn't
particularly invasive and I've hammered the SSD with it over night with
a combination of IOs and smart commands, it's solid.

But if you prefer waiting for 5.4, no worries.

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Fri, 2019-07-19 at 23:51 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2019-07-19 at 14:28 +0200, Christoph Hellwig wrote:
> > Yikes, that things looks worse and worse.  I think at this point
> > we'll
> > have to defer the support to 5.4 unfortunately as it is getting more
> > and more involved..
> 
> Well, at least v3 of that patch, thanks to Damien's idea, isn't
> particularly invasive and I've hammered the SSD with it over night with
> a combination of IOs and smart commands, it's solid.
> 
> But if you prefer waiting for 5.4, no worries.

BTW. It's been solid for 3 days now, so I think that was the last of it
(famous last words...)

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Ming Lei]

On Fri, Jul 19, 2019 at 1:31 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Fri, 19 Jul 2019 15:03:06 +1000
> Subject:
>
> Another issue with the Apple T2 based 2018 controllers seem to be
> that they blow up (and shut the machine down) if there's a tag
> collision between the IO queue and the Admin queue.
>
> My suspicion is that they use our tags for their internal tracking
> and don't mix them with the queue id. They also seem to not like
> when tags go beyond the IO queue depth, ie 128 tags.
>
> This adds a quirk that marks tags 0..31 of the IO queue reserved
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> Thanks Damien, reserved tags work and make this a lot simpler !
>
>  drivers/nvme/host/nvme.h |  5 +++++
>  drivers/nvme/host/pci.c  | 19 ++++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index ced0e0a7e039..8732da6df555 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -102,6 +102,11 @@ enum nvme_quirks {
>          * Use non-standard 128 bytes SQEs.
>          */
>         NVME_QUIRK_128_BYTES_SQES               = (1 << 11),
> +
> +       /*
> +        * Prevent tag overlap between queues
> +        */
> +       NVME_QUIRK_SHARED_TAGS                  = (1 << 12),
>  };
>
>  /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7088971d4c42..fc74395a028b 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2106,6 +2106,14 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
>         unsigned long size;
>
>         nr_io_queues = max_io_queues();
> +
> +       /*
> +        * If tags are shared with admin queue (Apple bug), then
> +        * make sure we only use one IO queue.
> +        */
> +       if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
> +               nr_io_queues = 1;
> +
>         result = nvme_set_queue_count(&dev->ctrl, &nr_io_queues);
>         if (result < 0)
>                 return result;
> @@ -2278,6 +2286,14 @@ static int nvme_dev_add(struct nvme_dev *dev)
>                 dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
>                 dev->tagset.driver_data = dev;
>
> +               /*
> +                * Some Apple controllers requires tags to be unique
> +                * across admin and IO queue, so reserve the first 32
> +                * tags of the IO queue.
> +                */
> +               if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
> +                       dev->tagset.reserved_tags = NVME_AQ_DEPTH;
> +
>                 ret = blk_mq_alloc_tag_set(&dev->tagset);
>                 if (ret) {
>                         dev_warn(dev->ctrl.device,
> @@ -3057,7 +3073,8 @@ static const struct pci_device_id nvme_id_table[] = {
>         { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
>         { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
>                 .driver_data = NVME_QUIRK_SINGLE_VECTOR |
> -                               NVME_QUIRK_128_BYTES_SQES },
> +                               NVME_QUIRK_128_BYTES_SQES |
> +                               NVME_QUIRK_SHARED_TAGS },
>         { 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, nvme_id_table);

Looks fine for me:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks,
Ming Lei

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Keith Busch]

On Fri, Jul 19, 2019 at 03:31:02PM +1000, Benjamin Herrenschmidt wrote:
> From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Fri, 19 Jul 2019 15:03:06 +1000
> Subject: 
> 
> Another issue with the Apple T2 based 2018 controllers seem to be
> that they blow up (and shut the machine down) if there's a tag
> collision between the IO queue and the Admin queue.
> 
> My suspicion is that they use our tags for their internal tracking
> and don't mix them with the queue id. They also seem to not like
> when tags go beyond the IO queue depth, ie 128 tags.
> 
> This adds a quirk that marks tags 0..31 of the IO queue reserved
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---

One problem is that we've an nvme parameter, io_queue_depth, that a user
could set to something less than 32, and then you won't be able to do
any IO. I'd recommend enforce the admin queue to QD1 for this device so
that you have more potential IO tags.

 
> Thanks Damien, reserved tags work and make this a lot simpler !
> 
>  drivers/nvme/host/nvme.h |  5 +++++
>  drivers/nvme/host/pci.c  | 19 ++++++++++++++++++-
>  2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index ced0e0a7e039..8732da6df555 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -102,6 +102,11 @@ enum nvme_quirks {
>  	 * Use non-standard 128 bytes SQEs.
>  	 */
>  	NVME_QUIRK_128_BYTES_SQES		= (1 << 11),
> +
> +	/*
> +	 * Prevent tag overlap between queues
> +	 */
> +	NVME_QUIRK_SHARED_TAGS                  = (1 << 12),
>  };
>  
>  /*
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 7088971d4c42..fc74395a028b 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2106,6 +2106,14 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
>  	unsigned long size;
>  
>  	nr_io_queues = max_io_queues();
> +
> +	/*
> +	 * If tags are shared with admin queue (Apple bug), then
> +	 * make sure we only use one IO queue.
> +	 */
> +	if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
> +		nr_io_queues = 1;
> +
>  	result = nvme_set_queue_count(&dev->ctrl, &nr_io_queues);
>  	if (result < 0)
>  		return result;
> @@ -2278,6 +2286,14 @@ static int nvme_dev_add(struct nvme_dev *dev)
>  		dev->tagset.flags = BLK_MQ_F_SHOULD_MERGE;
>  		dev->tagset.driver_data = dev;
>  
> +		/*
> +		 * Some Apple controllers requires tags to be unique
> +		 * across admin and IO queue, so reserve the first 32
> +		 * tags of the IO queue.
> +		 */
> +		if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS)
> +			dev->tagset.reserved_tags = NVME_AQ_DEPTH;
> +
>  		ret = blk_mq_alloc_tag_set(&dev->tagset);
>  		if (ret) {
>  			dev_warn(dev->ctrl.device,
> @@ -3057,7 +3073,8 @@ static const struct pci_device_id nvme_id_table[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2005),
>  		.driver_data = NVME_QUIRK_SINGLE_VECTOR |
> -				NVME_QUIRK_128_BYTES_SQES },
> +				NVME_QUIRK_128_BYTES_SQES |
> +				NVME_QUIRK_SHARED_TAGS },
>  	{ 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, nvme_id_table);
> 
> 

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Tue, 2019-07-30 at 09:30 -0600, Keith Busch wrote:
> On Fri, Jul 19, 2019 at 03:31:02PM +1000, Benjamin Herrenschmidt wrote:
> > From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Fri, 19 Jul 2019 15:03:06 +1000
> > Subject: 
> > 
> > Another issue with the Apple T2 based 2018 controllers seem to be
> > that they blow up (and shut the machine down) if there's a tag
> > collision between the IO queue and the Admin queue.
> > 
> > My suspicion is that they use our tags for their internal tracking
> > and don't mix them with the queue id. They also seem to not like
> > when tags go beyond the IO queue depth, ie 128 tags.
> > 
> > This adds a quirk that marks tags 0..31 of the IO queue reserved
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> 
> One problem is that we've an nvme parameter, io_queue_depth, that a user
> could set to something less than 32, and then you won't be able to do
> any IO. I'd recommend enforce the admin queue to QD1 for this device so
> that you have more potential IO tags.

Makes sense, I don't think we care much about the number of admin tags 
on these devices.

I'm travelling, not sure I'll be able to respin & test before next
week.

Thanks.

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Tue, 2019-07-30 at 09:30 -0600, Keith Busch wrote:
> On Fri, Jul 19, 2019 at 03:31:02PM +1000, Benjamin Herrenschmidt wrote:
> > From 8dcba2ef5b1466b023b88b4eca463b30de78d9eb Mon Sep 17 00:00:00 2001
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Fri, 19 Jul 2019 15:03:06 +1000
> > Subject: 
> > 
> > Another issue with the Apple T2 based 2018 controllers seem to be
> > that they blow up (and shut the machine down) if there's a tag
> > collision between the IO queue and the Admin queue.
> > 
> > My suspicion is that they use our tags for their internal tracking
> > and don't mix them with the queue id. They also seem to not like
> > when tags go beyond the IO queue depth, ie 128 tags.
> > 
> > This adds a quirk that marks tags 0..31 of the IO queue reserved
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> 
> One problem is that we've an nvme parameter, io_queue_depth, that a user
> could set to something less than 32, and then you won't be able to do
> any IO. I'd recommend enforce the admin queue to QD1 for this device so
> that you have more potential IO tags.

So I had a look and it's not that trivial. I would have to change
a few things that use constants for the admin queue depth, such as
the AEN tag etc...

For such a special case, I am tempted instead to do the much simpler:

	if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS) {
		if (dev->q_depth < (NVME_AQ_DEPTH + 2))
			dev->q_depth = NVME_AQ_DEPTH + 2;
	}

In nvme_pci_enable() next to the existing q_depth hackery for other
controllers.

Thoughts ?

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Tue, 2019-07-30 at 13:28 -0700, Benjamin Herrenschmidt wrote:
> > One problem is that we've an nvme parameter, io_queue_depth, that a user
> > could set to something less than 32, and then you won't be able to do
> > any IO. I'd recommend enforce the admin queue to QD1 for this device so
> > that you have more potential IO tags.
> 
> So I had a look and it's not that trivial. I would have to change
> a few things that use constants for the admin queue depth, such as
> the AEN tag etc...
> 
> For such a special case, I am tempted instead to do the much simpler:
> 
>         if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS) {
>                 if (dev->q_depth < (NVME_AQ_DEPTH + 2))
>                         dev->q_depth = NVME_AQ_DEPTH + 2;
>         }
> 
> In nvme_pci_enable() next to the existing q_depth hackery for other
> controllers.
> 
> Thoughts ?

Ping ? I had another look today and I don't feel like mucking around
with all the AQ size logic, AEN magic tag etc... just for that sake of
that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
to make much of a difference in practice anyway.

But if you feel strongly about it, then I'll implement the "proper" way
sometimes this week, adding a way to shrink the AQ down to something
like 3 (one admin request, one async event (AEN), and the empty slot)
by making a bunch of the constants involved variables instead.

This leas to a question: Wouldn't be be nicer/cleaner to make AEN be
tag 0 of the AQ ? That way we just include it as reserved tag ? Not a
huge different from what we do now, just a thought.

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Keith Busch]

On Mon, Aug 05, 2019 at 04:49:23PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2019-07-30 at 13:28 -0700, Benjamin Herrenschmidt wrote:
> > > One problem is that we've an nvme parameter, io_queue_depth, that a user
> > > could set to something less than 32, and then you won't be able to do
> > > any IO. I'd recommend enforce the admin queue to QD1 for this device so
> > > that you have more potential IO tags.
> > 
> > So I had a look and it's not that trivial. I would have to change
> > a few things that use constants for the admin queue depth, such as
> > the AEN tag etc...
> > 
> > For such a special case, I am tempted instead to do the much simpler:
> > 
> >         if (dev->ctrl.quirks & NVME_QUIRK_SHARED_TAGS) {
> >                 if (dev->q_depth < (NVME_AQ_DEPTH + 2))
> >                         dev->q_depth = NVME_AQ_DEPTH + 2;
> >         }
> > 
> > In nvme_pci_enable() next to the existing q_depth hackery for other
> > controllers.
> > 
> > Thoughts ?
> 
> Ping ? I had another look today and I don't feel like mucking around
> with all the AQ size logic, AEN magic tag etc... just for that sake of
> that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
> to make much of a difference in practice anyway.
> 
> But if you feel strongly about it, then I'll implement the "proper" way
> sometimes this week, adding a way to shrink the AQ down to something
> like 3 (one admin request, one async event (AEN), and the empty slot)
> by making a bunch of the constants involved variables instead.

I don't feel too strongly about it. I think your patch is fine, so

Acked-by: Keith Busch <keith.busch@intel.com>

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Sagi Grimberg]

>> Ping ? I had another look today and I don't feel like mucking around
>> with all the AQ size logic, AEN magic tag etc... just for that sake of
>> that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
>> to make much of a difference in practice anyway.
>>
>> But if you feel strongly about it, then I'll implement the "proper" way
>> sometimes this week, adding a way to shrink the AQ down to something
>> like 3 (one admin request, one async event (AEN), and the empty slot)
>> by making a bunch of the constants involved variables instead.
> 
> I don't feel too strongly about it. I think your patch is fine, so
> 
> Acked-by: Keith Busch <keith.busch@intel.com>

Should we pick this up for 5.3-rc?

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Keith Busch]

On Mon, Aug 05, 2019 at 11:27:54AM -0700, Sagi Grimberg wrote:
> 
> > > Ping ? I had another look today and I don't feel like mucking around
> > > with all the AQ size logic, AEN magic tag etc... just for that sake of
> > > that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
> > > to make much of a difference in practice anyway.
> > > 
> > > But if you feel strongly about it, then I'll implement the "proper" way
> > > sometimes this week, adding a way to shrink the AQ down to something
> > > like 3 (one admin request, one async event (AEN), and the empty slot)
> > > by making a bunch of the constants involved variables instead.
> > 
> > I don't feel too strongly about it. I think your patch is fine, so
> > 
> > Acked-by: Keith Busch <keith.busch@intel.com>
> 
> Should we pick this up for 5.3-rc?

Probably not. While I don't think this is a risky patch set, it's not
a bug fix for anything we introduced during the merge window. Christoph
also stated he wanted this to go in the 5.4 merge window.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Jens Axboe]

On 8/5/19 11:27 AM, Sagi Grimberg wrote:
> 
>>> Ping ? I had another look today and I don't feel like mucking around
>>> with all the AQ size logic, AEN magic tag etc... just for that sake of
>>> that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
>>> to make much of a difference in practice anyway.
>>>
>>> But if you feel strongly about it, then I'll implement the "proper" way
>>> sometimes this week, adding a way to shrink the AQ down to something
>>> like 3 (one admin request, one async event (AEN), and the empty slot)
>>> by making a bunch of the constants involved variables instead.
>>
>> I don't feel too strongly about it. I think your patch is fine, so
>>
>> Acked-by: Keith Busch <keith.busch@intel.com>
> 
> Should we pick this up for 5.3-rc?

No, it's not a regression fix. Queue it up for 5.4 instead.

-- 
Jens Axboe

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Sagi Grimberg]

>>>> Ping ? I had another look today and I don't feel like mucking around
>>>> with all the AQ size logic, AEN magic tag etc... just for that sake of
>>>> that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
>>>> to make much of a difference in practice anyway.
>>>>
>>>> But if you feel strongly about it, then I'll implement the "proper" way
>>>> sometimes this week, adding a way to shrink the AQ down to something
>>>> like 3 (one admin request, one async event (AEN), and the empty slot)
>>>> by making a bunch of the constants involved variables instead.
>>>
>>> I don't feel too strongly about it. I think your patch is fine, so
>>>
>>> Acked-by: Keith Busch <keith.busch@intel.com>
>>
>> Should we pick this up for 5.3-rc?
> 
> No, it's not a regression fix. Queue it up for 5.4 instead.

OK, will queue it up for nvme-5.4

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Sagi Grimberg]

>>>>> Ping ? I had another look today and I don't feel like mucking around
>>>>> with all the AQ size logic, AEN magic tag etc... just for that sake of
>>>>> that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
>>>>> to make much of a difference in practice anyway.
>>>>>
>>>>> But if you feel strongly about it, then I'll implement the "proper" 
>>>>> way
>>>>> sometimes this week, adding a way to shrink the AQ down to something
>>>>> like 3 (one admin request, one async event (AEN), and the empty slot)
>>>>> by making a bunch of the constants involved variables instead.
>>>>
>>>> I don't feel too strongly about it. I think your patch is fine, so
>>>>
>>>> Acked-by: Keith Busch <keith.busch@intel.com>
>>>
>>> Should we pick this up for 5.3-rc?
>>
>> No, it's not a regression fix. Queue it up for 5.4 instead.
> 
> OK, will queue it up for nvme-5.4

Doesn't apply..

Ben, can you please respin a patch that applies on nvme-5.4?

http://git.infradead.org/nvme.git/shortlog/refs/heads/nvme-5.4

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Mon, 2019-08-05 at 07:49 -0600, Keith Busch wrote:
> > Ping ? I had another look today and I don't feel like mucking around
> > with all the AQ size logic, AEN magic tag etc... just for that sake of
> > that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
> > to make much of a difference in practice anyway.
> > 
> > But if you feel strongly about it, then I'll implement the "proper" way
> > sometimes this week, adding a way to shrink the AQ down to something
> > like 3 (one admin request, one async event (AEN), and the empty slot)
> > by making a bunch of the constants involved variables instead.
> 
> I don't feel too strongly about it. I think your patch is fine, so
> 
> Acked-by: Keith Busch <keith.busch@intel.com>

Thanks, I'll fold the test and respin this week.

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Mon, 2019-08-05 at 11:27 -0700, Sagi Grimberg wrote:
> > > Ping ? I had another look today and I don't feel like mucking
> > > around
> > > with all the AQ size logic, AEN magic tag etc... just for that
> > > sake of
> > > that Apple gunk. I'm happy to have it give up IO tags, it doesn't
> > > seem
> > > to make much of a difference in practice anyway.
> > > 
> > > But if you feel strongly about it, then I'll implement the
> > > "proper" way
> > > sometimes this week, adding a way to shrink the AQ down to
> > > something
> > > like 3 (one admin request, one async event (AEN), and the empty
> > > slot)
> > > by making a bunch of the constants involved variables instead.
> > 
> > I don't feel too strongly about it. I think your patch is fine, so
> > 
> > Acked-by: Keith Busch <keith.busch@intel.com>
> 
> Should we pick this up for 5.3-rc?

Well, if you are talking about just this patch, it's not needed unless
you also merge the rest of the Apple T2 support patches, which some
people I'm sure would like but I think Christoph is a bit cold feet
about (I don't care either way).

If you are talking about the whole series, give me a day or two to
respin with the added check that Keith requested (I believe his ack is
for a respin with the check so setting the IO queue too small doesn't
kill the driver).

Cheers,
Ben.

Re: [PATCH v3] nvme-pci: Support shared tags across queues for Apple 2018 controllers

[Benjamin Herrenschmidt]

On Mon, 2019-08-05 at 13:07 -0700, Sagi Grimberg wrote:
> > > > > > Ping ? I had another look today and I don't feel like mucking around
> > > > > > with all the AQ size logic, AEN magic tag etc... just for that sake of
> > > > > > that Apple gunk. I'm happy to have it give up IO tags, it doesn't seem
> > > > > > to make much of a difference in practice anyway.
> > > > > > 
> > > > > > But if you feel strongly about it, then I'll implement the "proper" 
> > > > > > way
> > > > > > sometimes this week, adding a way to shrink the AQ down to something
> > > > > > like 3 (one admin request, one async event (AEN), and the empty slot)
> > > > > > by making a bunch of the constants involved variables instead.
> > > > > 
> > > > > I don't feel too strongly about it. I think your patch is fine, so
> > > > > 
> > > > > Acked-by: Keith Busch <keith.busch@intel.com>
> > > > 
> > > > Should we pick this up for 5.3-rc?
> > > 
> > > No, it's not a regression fix. Queue it up for 5.4 instead.
> > 
> > OK, will queue it up for nvme-5.4
> 
> Doesn't apply..
> 
> Ben, can you please respin a patch that applies on nvme-5.4?
> 
> http://git.infradead.org/nvme.git/shortlog/refs/heads/nvme-5.4

Sure, will do in the next couple of days !

Cheers,
Ben.