[PATCH 0/2] fix sil24 regression and muddy the ata tag allocation waters

[PATCH 0/2] fix sil24 regression and muddy the ata tag allocation waters

[Dan Williams]

The last we left the fix for the reported sil24 regression [1], Tejun
had asked that I simplify the patch to just a flag [2].

Patch1 incorporates that feedback.  Patch2 tries to make the ata tag
allocator implementation slightly better than its current worst case
implementation.  I'm fine with replacing it with something better from
the block layer, but that implementation had better address the quirk
that patch1 does.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=87101
[2]: http://marc.info/?l=linux-ide&m=141511124009903&w=2

---

Dan Williams (2):
      libata: allow sata_sil24 to opt-out of tag ordered submission
      libata: micro-optimize tag allocation


 drivers/ata/libahci.c     |    2 +-
 drivers/ata/libata-core.c |   56 ++++++++++++++++++++++++++-------------------
 drivers/ata/libata-sff.c  |    2 +-
 drivers/ata/sata_sil24.c  |    2 +-
 include/linux/libata.h    |    4 ++-
 5 files changed, 37 insertions(+), 29 deletions(-)

[PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Dan Williams]

Ronny reports: https://bugzilla.kernel.org/show_bug.cgi?id=87101
    "Since commit 8a4aeec8d "libata/ahci: accommodate tag ordered
    controllers" the access to the harddisk on the first SATA-port is
    failing on its first access. The access to the harddisk on the
    second port is working normal.

    When reverting the above commit, access to both harddisks is working
    fine again."

Maintain tag ordered submission as the default, but allow sata_sil24 to
continue with the old behavior.

Cc: <stable@vger.kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Reported-by: Ronny Hegewald <Ronny.Hegewald@online.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libata-core.c |    5 ++++-
 drivers/ata/sata_sil24.c  |    2 +-
 include/linux/libata.h    |    1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 5c84fb5c3372..e2085d4b5b93 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4748,7 +4748,10 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 		return NULL;
 
 	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
-		tag = tag < max_queue ? tag : 0;
+		if (ap->flags & ATA_FLAG_LOWTAG)
+			tag = i;
+		else
+			tag = tag < max_queue ? tag : 0;
 
 		/* the last tag is reserved for internal command. */
 		if (tag == ATA_TAG_INTERNAL)
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index d81b20ddb527..ea655949023f 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -246,7 +246,7 @@ enum {
 	/* host flags */
 	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
 				  ATA_FLAG_NCQ | ATA_FLAG_ACPI_SATA |
-				  ATA_FLAG_AN | ATA_FLAG_PMP,
+				  ATA_FLAG_AN | ATA_FLAG_PMP | ATA_FLAG_LOWTAG,
 	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
 
 	IRQ_STAT_4PORTS		= 0xf,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2d182413b1db..aba87a3f60bc 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -231,6 +231,7 @@ enum {
 	ATA_FLAG_SW_ACTIVITY	= (1 << 22), /* driver supports sw activity
 					      * led */
 	ATA_FLAG_NO_DIPM	= (1 << 23), /* host not happy with DIPM */
+	ATA_FLAG_LOWTAG		= (1 << 24), /* host wants lowest available tag */
 
 	/* bits 24:31 of ap->flags are reserved for LLD specific flags */
 

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Sergei Shtylyov]

Hello.

On 1/17/2015 2:13 AM, Dan Williams wrote:

> Ronny reports: https://bugzilla.kernel.org/show_bug.cgi?id=87101
>      "Since commit 8a4aeec8d "libata/ahci: accommodate tag ordered
>      controllers" the access to the harddisk on the first SATA-port is
>      failing on its first access. The access to the harddisk on the
>      second port is working normal.

>      When reverting the above commit, access to both harddisks is working
>      fine again."

> Maintain tag ordered submission as the default, but allow sata_sil24 to
> continue with the old behavior.

> Cc: <stable@vger.kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Reported-by: Ronny Hegewald <Ronny.Hegewald@online.de>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   drivers/ata/libata-core.c |    5 ++++-
>   drivers/ata/sata_sil24.c  |    2 +-
>   include/linux/libata.h    |    1 +
>   3 files changed, 6 insertions(+), 2 deletions(-)

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 5c84fb5c3372..e2085d4b5b93 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4748,7 +4748,10 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
>   		return NULL;
>
>   	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
> -		tag = tag < max_queue ? tag : 0;
> +		if (ap->flags & ATA_FLAG_LOWTAG)
> +			tag = i;
> +		else
> +			tag = tag < max_queue ? tag : 0;

    Ugh, this is clear abuse of the ?: operator... Why not simply:

		else if (tag >= max_queue)
			tag = 0;

[...]

MBR, Sergei

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Sergei Shtylyov]

On 01/17/2015 09:35 PM, Dan Williams wrote:

>  >> Ronny reports: https://bugzilla.kernel.org/show_bug.cgi?id=87101
>  >>      "Since commit 8a4aeec8d "libata/ahci: accommodate tag ordered
>  >>      controllers" the access to the harddisk on the first SATA-port is
>  >>      failing on its first access. The access to the harddisk on the
>  >>      second port is working normal.

>  >>      When reverting the above commit, access to both harddisks is working
>  >>      fine again."

>  >> Maintain tag ordered submission as the default, but allow sata_sil24 to
>  >> continue with the old behavior.

>  >> Cc: <stable@vger.kernel.org <mailto:stable@vger.kernel.org>>
>  >> Cc: Tejun Heo <tj@kernel.org <mailto:tj@kernel.org>>
>  >> Reported-by: Ronny Hegewald <Ronny.Hegewald@online.de>
>  >> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>  >> ---
>  >>   drivers/ata/libata-core.c |    5 ++++-
>  >>   drivers/ata/sata_sil24.c  |    2 +-
>  >>   include/linux/libata.h    |    1 +
>  >>   3 files changed, 6 insertions(+), 2 deletions(-)

>  >> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>  >> index 5c84fb5c3372..e2085d4b5b93 100644
>  >> --- a/drivers/ata/libata-core.c
>  >> +++ b/drivers/ata/libata-core.c
>  >> @@ -4748,7 +4748,10 @@ static struct ata_queued_cmd *ata_qc_new(struct
> ata_port *ap)
>  >>                 return NULL;
>  >>
>  >>         for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
>  >> -               tag = tag < max_queue ? tag : 0;
>  >> +               if (ap->flags & ATA_FLAG_LOWTAG)
>  >> +                       tag = i;
>  >> +               else
>  >> +                       tag = tag < max_queue ? tag : 0;

>  >    Ugh, this is clear abuse of the ?: operator... Why not simply:

>  >                 else if (tag >= max_queue)
>  >                         tag = 0;

> "Abuse"!? Let's just call it "creative differences adding in a minimal quirk
> while neglecting to refactor" ;-).

    In fact, the old code had the same abuse, I didn't notice that...

> Sure, that's cleaner.

    Thanks. :-)

MBR, Sergei

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Tejun Heo]

On Sat, Jan 17, 2015 at 01:59:42PM +0300, Sergei Shtylyov wrote:
> >  	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
> >-		tag = tag < max_queue ? tag : 0;
> >+		if (ap->flags & ATA_FLAG_LOWTAG)
> >+			tag = i;
> >+		else
> >+			tag = tag < max_queue ? tag : 0;
> 
>    Ugh, this is clear abuse of the ?: operator... Why not simply:
> 
> 		else if (tag >= max_queue)
> 			tag = 0;

Why is that a clear abuse?  Seems like a pretty typical use to me.

Thanks.

-- 
tejun

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Sergei Shtylyov]

Hello.

On 1/19/2015 5:12 PM, Tejun Heo wrote:

>>>   	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
>>> -		tag = tag < max_queue ? tag : 0;
>>> +		if (ap->flags & ATA_FLAG_LOWTAG)
>>> +			tag = i;
>>> +		else
>>> +			tag = tag < max_queue ? tag : 0;

>>     Ugh, this is clear abuse of the ?: operator... Why not simply:

>> 		else if (tag >= max_queue)
>> 			tag = 0;

> Why is that a clear abuse?  Seems like a pretty typical use to me.

    This is my first reaction to statements like 'x = x < M ? x : 0'. I really
prefer two-liners with *if* to them.

> Thanks.

MBR, Sergei

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Tejun Heo]

On Mon, Jan 19, 2015 at 05:24:30PM +0300, Sergei Shtylyov wrote:
> >Why is that a clear abuse?  Seems like a pretty typical use to me.
> 
>    This is my first reaction to statements like 'x = x < M ? x : 0'. I really
> prefer two-liners with *if* to them.

But that doesn't justify the "clear abuse" part at all.  I get that
different people may have different preferences but that's not what
you expressed.

Thanks.

-- 
tejun

Re: [PATCH 1/2] libata: allow sata_sil24 to opt-out of tag ordered submission

[Tejun Heo]

On Fri, Jan 16, 2015 at 03:13:02PM -0800, Dan Williams wrote:
> Ronny reports: https://bugzilla.kernel.org/show_bug.cgi?id=87101
>     "Since commit 8a4aeec8d "libata/ahci: accommodate tag ordered
>     controllers" the access to the harddisk on the first SATA-port is
>     failing on its first access. The access to the harddisk on the
>     second port is working normal.
> 
>     When reverting the above commit, access to both harddisks is working
>     fine again."
> 
> Maintain tag ordered submission as the default, but allow sata_sil24 to
> continue with the old behavior.
> 
> Cc: <stable@vger.kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Reported-by: Ronny Hegewald <Ronny.Hegewald@online.de>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Applied to libata/for-3.18-fixes.

Thanks.

-- 
tejun

[PATCH 2/2] libata: micro-optimize tag allocation

[Dan Williams]

Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
almost a worst case implementation."  Previously I thought SATA mmio
latency dominated performance profiles, but as Tejun notes:

  "Hmmm... one problem with the existing tag allocator in ata is that
   it's not very efficient which actually shows up in profile when libata
   is used with a very zippy SSD.  Given that ata needs a different
   allocation policies anyway maybe the right thing to do is making the
   existing allocator suck less."

So replace it with a naive enhancement that also supports the existing
quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
[2].

[1]: http://marc.info/?l=linux-ide&m=142137195324687&w=2
[2]: https://bugzilla.kernel.org/show_bug.cgi?id=87101

Cc: Jens Axboe <axboe@fb.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Shaohua Li <shli@fb.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/ata/libahci.c     |    2 +-
 drivers/ata/libata-core.c |   59 ++++++++++++++++++++++++---------------------
 drivers/ata/libata-sff.c  |    2 +-
 include/linux/libata.h    |    3 +-
 4 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 97683e45ab04..5b8983738172 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2423,7 +2423,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,
 {
 	int i, rc;
 
-	rc = ata_host_start(host);
+	rc = ata_host_start(host, sht->can_queue);
 	if (rc)
 		return rc;
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e2085d4b5b93..71415819b072 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1585,7 +1585,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev,
 	else
 		tag = 0;
 
-	if (test_and_set_bit(tag, &ap->qc_allocated))
+	if (__test_and_set_bit(tag, &ap->qc_allocated))
 		BUG();
 	qc = __ata_qc_from_tag(ap, tag);
 
@@ -4740,29 +4740,32 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
 static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap)
 {
 	struct ata_queued_cmd *qc = NULL;
-	unsigned int max_queue = ap->host->n_tags;
-	unsigned int i, tag;
+	int i;
 
 	/* no command while frozen */
 	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
 		return NULL;
 
-	for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) {
-		if (ap->flags & ATA_FLAG_LOWTAG)
-			tag = i;
-		else
-			tag = tag < max_queue ? tag : 0;
-
-		/* the last tag is reserved for internal command. */
-		if (tag == ATA_TAG_INTERNAL)
+	/*
+	 * Find first zero bit in qc_allocated starting from ->last_tag,
+	 * unless ATA_FLAG_LOWTAG is set.  In the ATA_FLAG_LOWTAG case
+	 * start searching from zero.
+	 */
+	for (i = !(ap->flags & ATA_FLAG_LOWTAG); i >= 0; i--) {
+		unsigned long qc_allocated = ap->qc_allocated, tag;
+		int last_tag = ap->last_tag * i;
+
+		/* note, ap->qc_allocated protected by ap->lock */
+		qc_allocated |= ~((~0UL >> last_tag) << last_tag);
+		qc_allocated |= (1 << ATA_TAG_INTERNAL);
+		if (qc_allocated == ~0UL)
 			continue;
-
-		if (!test_and_set_bit(tag, &ap->qc_allocated)) {
-			qc = __ata_qc_from_tag(ap, tag);
-			qc->tag = tag;
-			ap->last_tag = tag;
-			break;
-		}
+		tag = ffz(qc_allocated);
+		__set_bit(tag, &ap->qc_allocated);
+		qc = __ata_qc_from_tag(ap, tag);
+		qc->tag = tag;
+		ap->last_tag = tag;
+		break;
 	}
 
 	return qc;
@@ -4815,7 +4818,7 @@ void ata_qc_free(struct ata_queued_cmd *qc)
 	tag = qc->tag;
 	if (likely(ata_tag_valid(tag))) {
 		qc->tag = ATA_TAG_POISON;
-		clear_bit(tag, &ap->qc_allocated);
+		__clear_bit(tag, &ap->qc_allocated);
 	}
 }
 
@@ -5962,20 +5965,25 @@ static void ata_finalize_port_ops(struct ata_port_operations *ops)
  *	RETURNS:
  *	0 if all ports are started successfully, -errno otherwise.
  */
-int ata_host_start(struct ata_host *host)
+int ata_host_start(struct ata_host *host, int can_queue)
 {
-	int have_stop = 0;
+	int i, rc, n_tags, have_stop = 0;
+	unsigned long qc_allocated;
 	void *start_dr = NULL;
-	int i, rc;
 
 	if (host->flags & ATA_HOST_STARTED)
 		return 0;
 
 	ata_finalize_port_ops(host->ops);
 
+	n_tags = clamp(can_queue, 1, ATA_MAX_QUEUE - 1);
+	qc_allocated = (~0UL >> n_tags) << n_tags;
+	clear_bit(ATA_TAG_INTERNAL, &qc_allocated);
+
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
+		ap->qc_allocated = qc_allocated;
 		ata_finalize_port_ops(ap->ops);
 
 		if (!host->ops && !ata_port_is_dummy(ap))
@@ -6038,7 +6046,6 @@ void ata_host_init(struct ata_host *host, struct device *dev,
 {
 	spin_lock_init(&host->lock);
 	mutex_init(&host->eh_mutex);
-	host->n_tags = ATA_MAX_QUEUE - 1;
 	host->dev = dev;
 	host->ops = ops;
 }
@@ -6120,8 +6127,6 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 {
 	int i, rc;
 
-	host->n_tags = clamp(sht->can_queue, 1, ATA_MAX_QUEUE - 1);
-
 	/* host must have been started */
 	if (!(host->flags & ATA_HOST_STARTED)) {
 		dev_err(host->dev, "BUG: trying to register unstarted host\n");
@@ -6136,7 +6141,7 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	for (i = host->n_ports; host->ports[i]; i++)
 		kfree(host->ports[i]);
 
-	/* give ports names and add SCSI hosts */
+	/* give ports names and add init tag allocation */
 	for (i = 0; i < host->n_ports; i++) {
 		host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
 		host->ports[i]->local_port_no = i + 1;
@@ -6227,7 +6232,7 @@ int ata_host_activate(struct ata_host *host, int irq,
 {
 	int i, rc;
 
-	rc = ata_host_start(host);
+	rc = ata_host_start(host, sht->can_queue);
 	if (rc)
 		return rc;
 
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index db90aa35cb71..575ea268ef2a 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2419,7 +2419,7 @@ int ata_pci_sff_activate_host(struct ata_host *host,
 	const char *drv_name = dev_driver_string(host->dev);
 	int legacy_mode = 0, rc;
 
-	rc = ata_host_start(host);
+	rc = ata_host_start(host, sht->can_queue);
 	if (rc)
 		return rc;
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index aba87a3f60bc..32f4ba80be67 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -595,7 +595,6 @@ struct ata_host {
 	struct device 		*dev;
 	void __iomem * const	*iomap;
 	unsigned int		n_ports;
-	unsigned int		n_tags;			/* nr of NCQ tags */
 	void			*private_data;
 	struct ata_port_operations *ops;
 	unsigned long		flags;
@@ -1111,7 +1110,7 @@ extern struct ata_host *ata_host_alloc(struct device *dev, int max_ports);
 extern struct ata_host *ata_host_alloc_pinfo(struct device *dev,
 			const struct ata_port_info * const * ppi, int n_ports);
 extern int ata_slave_link_init(struct ata_port *ap);
-extern int ata_host_start(struct ata_host *host);
+extern int ata_host_start(struct ata_host *host, int can_queue);
 extern int ata_host_register(struct ata_host *host,
 			     struct scsi_host_template *sht);
 extern int ata_host_activate(struct ata_host *host, int irq,

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Jens Axboe]

On 01/16/2015 04:13 PM, Dan Williams wrote:
> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
> almost a worst case implementation."  Previously I thought SATA mmio
> latency dominated performance profiles, but as Tejun notes:
> 
>   "Hmmm... one problem with the existing tag allocator in ata is that
>    it's not very efficient which actually shows up in profile when libata
>    is used with a very zippy SSD.  Given that ata needs a different
>    allocation policies anyway maybe the right thing to do is making the
>    existing allocator suck less."
> 
> So replace it with a naive enhancement that also supports the existing
> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
> [2].

That's trivial to do, it's just always having '0' in the cache and
that's where the search would start.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Dan Williams]

On Fri, Jan 16, 2015 at 3:17 PM, Jens Axboe <axboe@fb.com> wrote:
> On 01/16/2015 04:13 PM, Dan Williams wrote:
>> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
>> almost a worst case implementation."  Previously I thought SATA mmio
>> latency dominated performance profiles, but as Tejun notes:
>>
>>   "Hmmm... one problem with the existing tag allocator in ata is that
>>    it's not very efficient which actually shows up in profile when libata
>>    is used with a very zippy SSD.  Given that ata needs a different
>>    allocation policies anyway maybe the right thing to do is making the
>>    existing allocator suck less."
>>
>> So replace it with a naive enhancement that also supports the existing
>> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
>> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
>> [2].
>
> That's trivial to do, it's just always having '0' in the cache and
> that's where the search would start.
>

Cool.

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Jens Axboe]

On 01/16/2015 04:17 PM, Jens Axboe wrote:
> On 01/16/2015 04:13 PM, Dan Williams wrote:
>> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
>> almost a worst case implementation."  Previously I thought SATA mmio
>> latency dominated performance profiles, but as Tejun notes:
>>
>>   "Hmmm... one problem with the existing tag allocator in ata is that
>>    it's not very efficient which actually shows up in profile when libata
>>    is used with a very zippy SSD.  Given that ata needs a different
>>    allocation policies anyway maybe the right thing to do is making the
>>    existing allocator suck less."
>>
>> So replace it with a naive enhancement that also supports the existing
>> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
>> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
>> [2].
> 
> That's trivial to do, it's just always having '0' in the cache and
> that's where the search would start.

BTW, I'm still puzzled by why sil24 fails with different tags, I can't
shake the feeling that some other bug is the cause of this.

-- 
Jens Axboe

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Dan Williams]

On Fri, Jan 16, 2015 at 3:21 PM, Jens Axboe <axboe@fb.com> wrote:
> On 01/16/2015 04:17 PM, Jens Axboe wrote:
>> On 01/16/2015 04:13 PM, Dan Williams wrote:
>>> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
>>> almost a worst case implementation."  Previously I thought SATA mmio
>>> latency dominated performance profiles, but as Tejun notes:
>>>
>>>   "Hmmm... one problem with the existing tag allocator in ata is that
>>>    it's not very efficient which actually shows up in profile when libata
>>>    is used with a very zippy SSD.  Given that ata needs a different
>>>    allocation policies anyway maybe the right thing to do is making the
>>>    existing allocator suck less."
>>>
>>> So replace it with a naive enhancement that also supports the existing
>>> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
>>> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
>>> [2].
>>
>> That's trivial to do, it's just always having '0' in the cache and
>> that's where the search would start.
>
> BTW, I'm still puzzled by why sil24 fails with different tags, I can't
> shake the feeling that some other bug is the cause of this.
>

Yes, a silicon bug :-).

Until "8a4aeec8d2d6 libata/ahci: accommodate tag ordered controllers"
never saw a different tag order besides the lowest available.  But
even "lowest available" will race with completions sometimes.

In any event, it's a puzzle that needs hardware to solve and I'm not
in a position to do anything outside of reverting the new behavior.

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Shaohua Li]

On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
> almost a worst case implementation."  Previously I thought SATA mmio
> latency dominated performance profiles, but as Tejun notes:
> 
>   "Hmmm... one problem with the existing tag allocator in ata is that
>    it's not very efficient which actually shows up in profile when libata
>    is used with a very zippy SSD.  Given that ata needs a different
>    allocation policies anyway maybe the right thing to do is making the
>    existing allocator suck less."
> 
> So replace it with a naive enhancement that also supports the existing
> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
> [2].
> 
> [1]: https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Dlinux-ide%26m%3D142137195324687%26w%3D2&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=cea914a77883ea668400a1f19621d1241cab16f6a4c0e68d0749de4c2448cdb1
> [2]: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=82d11bc720e9f0cbd0ad6aa7e8ece18315ac1e4f97cb02d49460893e5006228e

with my patch, we can fix this as:


diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index d81b20d..5242897 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -388,6 +388,7 @@ static struct scsi_host_template sil24_sht = {
 	.can_queue		= SIL24_MAX_CMDS,
 	.sg_tablesize		= SIL24_MAX_SGE,
 	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.tag_alloc_policy	= BLK_TAG_ALLOC_FIFO,
 };
 
 static struct ata_port_operations sil24_ops = {

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Dan Williams]

On Fri, Jan 16, 2015 at 3:31 PM, Shaohua Li <shli@fb.com> wrote:
> On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
>> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
>> almost a worst case implementation."  Previously I thought SATA mmio
>> latency dominated performance profiles, but as Tejun notes:
>>
>>   "Hmmm... one problem with the existing tag allocator in ata is that
>>    it's not very efficient which actually shows up in profile when libata
>>    is used with a very zippy SSD.  Given that ata needs a different
>>    allocation policies anyway maybe the right thing to do is making the
>>    existing allocator suck less."
>>
>> So replace it with a naive enhancement that also supports the existing
>> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
>> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
>> [2].
>>
>> [1]: https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Dlinux-ide%26m%3D142137195324687%26w%3D2&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=cea914a77883ea668400a1f19621d1241cab16f6a4c0e68d0749de4c2448cdb1
>> [2]: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=82d11bc720e9f0cbd0ad6aa7e8ece18315ac1e4f97cb02d49460893e5006228e
>
> with my patch, we can fix this as:
>
>
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index d81b20d..5242897 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -388,6 +388,7 @@ static struct scsi_host_template sil24_sht = {
>         .can_queue              = SIL24_MAX_CMDS,
>         .sg_tablesize           = SIL24_MAX_SGE,
>         .dma_boundary           = ATA_DMA_BOUNDARY,
> +       .tag_alloc_policy       = BLK_TAG_ALLOC_FIFO,
>  };
>
>  static struct ata_port_operations sil24_ops = {

Ok, thanks for that.

We still need patch1 as the minimal fix for the regression, agreed?

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Shaohua Li]

On Fri, Jan 16, 2015 at 03:49:07PM -0800, Dan Williams wrote:
> On Fri, Jan 16, 2015 at 3:31 PM, Shaohua Li <shli@fb.com> wrote:
> > On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
> >> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
> >> almost a worst case implementation."  Previously I thought SATA mmio
> >> latency dominated performance profiles, but as Tejun notes:
> >>
> >>   "Hmmm... one problem with the existing tag allocator in ata is that
> >>    it's not very efficient which actually shows up in profile when libata
> >>    is used with a very zippy SSD.  Given that ata needs a different
> >>    allocation policies anyway maybe the right thing to do is making the
> >>    existing allocator suck less."
> >>
> >> So replace it with a naive enhancement that also supports the existing
> >> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
> >> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
> >> [2].
> >>
> >> [1]: https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Dlinux-ide%26m%3D142137195324687%26w%3D2&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=cea914a77883ea668400a1f19621d1241cab16f6a4c0e68d0749de4c2448cdb1
> >> [2]: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=82d11bc720e9f0cbd0ad6aa7e8ece18315ac1e4f97cb02d49460893e5006228e
> >
> > with my patch, we can fix this as:
> >
> >
> > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> > index d81b20d..5242897 100644
> > --- a/drivers/ata/sata_sil24.c
> > +++ b/drivers/ata/sata_sil24.c
> > @@ -388,6 +388,7 @@ static struct scsi_host_template sil24_sht = {
> >         .can_queue              = SIL24_MAX_CMDS,
> >         .sg_tablesize           = SIL24_MAX_SGE,
> >         .dma_boundary           = ATA_DMA_BOUNDARY,
> > +       .tag_alloc_policy       = BLK_TAG_ALLOC_FIFO,
> >  };
> >
> >  static struct ata_port_operations sil24_ops = {
> 
> Ok, thanks for that.
> 
> We still need patch1 as the minimal fix for the regression, agreed?

The BLK_TAG_ALLOC_FIFO will make blk/blk-mq tag allocation allocates
lowest tag (eg, FIFO). So I thought it already fixes the sil24 bug, if I
understand the bug clearly.

Thanks,
Shaohua

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Dan Williams]

On Fri, Jan 16, 2015 at 3:55 PM, Shaohua Li <shli@fb.com> wrote:
> On Fri, Jan 16, 2015 at 03:49:07PM -0800, Dan Williams wrote:
>> On Fri, Jan 16, 2015 at 3:31 PM, Shaohua Li <shli@fb.com> wrote:
>> > On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
>> >> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
>> >> almost a worst case implementation."  Previously I thought SATA mmio
>> >> latency dominated performance profiles, but as Tejun notes:
>> >>
>> >>   "Hmmm... one problem with the existing tag allocator in ata is that
>> >>    it's not very efficient which actually shows up in profile when libata
>> >>    is used with a very zippy SSD.  Given that ata needs a different
>> >>    allocation policies anyway maybe the right thing to do is making the
>> >>    existing allocator suck less."
>> >>
>> >> So replace it with a naive enhancement that also supports the existing
>> >> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
>> >> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
>> >> [2].
>> >>
>> >> [1]: https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Dlinux-ide%26m%3D142137195324687%26w%3D2&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=cea914a77883ea668400a1f19621d1241cab16f6a4c0e68d0749de4c2448cdb1
>> >> [2]: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=82d11bc720e9f0cbd0ad6aa7e8ece18315ac1e4f97cb02d49460893e5006228e
>> >
>> > with my patch, we can fix this as:
>> >
>> >
>> > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
>> > index d81b20d..5242897 100644
>> > --- a/drivers/ata/sata_sil24.c
>> > +++ b/drivers/ata/sata_sil24.c
>> > @@ -388,6 +388,7 @@ static struct scsi_host_template sil24_sht = {
>> >         .can_queue              = SIL24_MAX_CMDS,
>> >         .sg_tablesize           = SIL24_MAX_SGE,
>> >         .dma_boundary           = ATA_DMA_BOUNDARY,
>> > +       .tag_alloc_policy       = BLK_TAG_ALLOC_FIFO,
>> >  };
>> >
>> >  static struct ata_port_operations sil24_ops = {
>>
>> Ok, thanks for that.
>>
>> We still need patch1 as the minimal fix for the regression, agreed?
>
> The BLK_TAG_ALLOC_FIFO will make blk/blk-mq tag allocation allocates
> lowest tag (eg, FIFO). So I thought it already fixes the sil24 bug, if I
> understand the bug clearly.
>

It looks like it does, but converting to blk-mq tagging is not
suitable for a -stable patch.

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Shaohua Li]

On Fri, Jan 16, 2015 at 03:59:32PM -0800, Dan Williams wrote:
> On Fri, Jan 16, 2015 at 3:55 PM, Shaohua Li <shli@fb.com> wrote:
> > On Fri, Jan 16, 2015 at 03:49:07PM -0800, Dan Williams wrote:
> >> On Fri, Jan 16, 2015 at 3:31 PM, Shaohua Li <shli@fb.com> wrote:
> >> > On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
> >> >> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
> >> >> almost a worst case implementation."  Previously I thought SATA mmio
> >> >> latency dominated performance profiles, but as Tejun notes:
> >> >>
> >> >>   "Hmmm... one problem with the existing tag allocator in ata is that
> >> >>    it's not very efficient which actually shows up in profile when libata
> >> >>    is used with a very zippy SSD.  Given that ata needs a different
> >> >>    allocation policies anyway maybe the right thing to do is making the
> >> >>    existing allocator suck less."
> >> >>
> >> >> So replace it with a naive enhancement that also supports the existing
> >> >> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
> >> >> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
> >> >> [2].
> >> >>
> >> >> [1]: https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Dlinux-ide%26m%3D142137195324687%26w%3D2&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=cea914a77883ea668400a1f19621d1241cab16f6a4c0e68d0749de4c2448cdb1
> >> >> [2]: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=5iyIBsm0XLUeBFFdEfPvaP6LL8KiN2TcuEGgJH6RzTc%3D%0A&s=82d11bc720e9f0cbd0ad6aa7e8ece18315ac1e4f97cb02d49460893e5006228e
> >> >
> >> > with my patch, we can fix this as:
> >> >
> >> >
> >> > diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> >> > index d81b20d..5242897 100644
> >> > --- a/drivers/ata/sata_sil24.c
> >> > +++ b/drivers/ata/sata_sil24.c
> >> > @@ -388,6 +388,7 @@ static struct scsi_host_template sil24_sht = {
> >> >         .can_queue              = SIL24_MAX_CMDS,
> >> >         .sg_tablesize           = SIL24_MAX_SGE,
> >> >         .dma_boundary           = ATA_DMA_BOUNDARY,
> >> > +       .tag_alloc_policy       = BLK_TAG_ALLOC_FIFO,
> >> >  };
> >> >
> >> >  static struct ata_port_operations sil24_ops = {
> >>
> >> Ok, thanks for that.
> >>
> >> We still need patch1 as the minimal fix for the regression, agreed?
> >
> > The BLK_TAG_ALLOC_FIFO will make blk/blk-mq tag allocation allocates
> > lowest tag (eg, FIFO). So I thought it already fixes the sil24 bug, if I
> > understand the bug clearly.
> >
> 
> It looks like it does, but converting to blk-mq tagging is not
> suitable for a -stable patch.

don't need converting to blk-mq, I added FIFO allocation for legacy tag
too, but right, it might not suitable for -stable

Re: [PATCH 2/2] libata: micro-optimize tag allocation

[Tejun Heo]

On Fri, Jan 16, 2015 at 03:13:08PM -0800, Dan Williams wrote:
> Jens notes, "libata tag allocator sucks. Like seriously sucks, it's
> almost a worst case implementation."  Previously I thought SATA mmio
> latency dominated performance profiles, but as Tejun notes:
> 
>   "Hmmm... one problem with the existing tag allocator in ata is that
>    it's not very efficient which actually shows up in profile when libata
>    is used with a very zippy SSD.  Given that ata needs a different
>    allocation policies anyway maybe the right thing to do is making the
>    existing allocator suck less."
> 
> So replace it with a naive enhancement that also supports the existing
> quirks.  Hopefully, soon to be replaced by Shaohua's patches [1], but
> those do not yet support the quirk needed by sil24 (ATA_FLAG_LOWTAG)
> [2].
> 
> [1]: http://marc.info/?l=linux-ide&m=142137195324687&w=2
> [2]: https://bugzilla.kernel.org/show_bug.cgi?id=87101
> 
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Shaohua Li <shli@fb.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hmmm... if libata using blk-tag isn't ugly, I think that prolly is the
better way to proceed.  Let's see how that develops.

Thanks.

-- 
tejun