| Message ID | 20150116233100.GA1281684@devbig257.prn2.facebook.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = {