Message ID | 1479306956-10292-6-git-send-email-tim.gardner@canonical.com |
---|---|
State | New |
Headers | show |
On Wed, Nov 16, 2016 at 07:35:56AM -0700, Tim Gardner wrote: > BugLink: http://bugs.launchpad.net/bugs/1637565 > > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > drivers/nvme/host/pci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 5256448..3931ceb 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, > enum dma_data_direction dma_dir = rq_data_dir(req) ? > DMA_TO_DEVICE : DMA_FROM_DEVICE; > int ret = BLK_MQ_RQ_QUEUE_ERROR; > + DEFINE_DMA_ATTRS(attrs); You also need this: dma_set_attr(DMA_ATTR_NO_WARN, &attrs); > > sg_init_table(iod->sg, req->nr_phys_segments); > iod->nents = blk_rq_map_sg(q, req, iod->sg); > @@ -631,7 +632,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, > > ret = BLK_MQ_RQ_QUEUE_BUSY; > if (!dma_map_sg_attrs(dev->dev, iod->sg, iod->nents, dma_dir, > - DMA_ATTR_NO_WARN)) > + &attrs)) > goto out; > > if (!nvme_setup_prps(dev, req, blk_rq_bytes(req))) > -- > 2.7.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 11/18/2016 06:36 AM, Seth Forshee wrote: > On Wed, Nov 16, 2016 at 07:35:56AM -0700, Tim Gardner wrote: >> BugLink: http://bugs.launchpad.net/bugs/1637565 >> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> --- >> drivers/nvme/host/pci.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c >> index 5256448..3931ceb 100644 >> --- a/drivers/nvme/host/pci.c >> +++ b/drivers/nvme/host/pci.c >> @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, >> enum dma_data_direction dma_dir = rq_data_dir(req) ? >> DMA_TO_DEVICE : DMA_FROM_DEVICE; >> int ret = BLK_MQ_RQ_QUEUE_ERROR; >> + DEFINE_DMA_ATTRS(attrs); > > You also need this: > > dma_set_attr(DMA_ATTR_NO_WARN, &attrs); > DMA_ATTR_NO_WARN was not introduced until v4.9-rc1. I think map_sg() expects something from the enumerated set 'enum dma_attr'. I did find an instance of its use in arch/powerpc/kernel/iommu.c where DMA_ATTR_NO_WARN is being queried. I think that is an erroneous backport on my part, but harmless. >> >> sg_init_table(iod->sg, req->nr_phys_segments); >> iod->nents = blk_rq_map_sg(q, req, iod->sg); >> @@ -631,7 +632,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, >> >> ret = BLK_MQ_RQ_QUEUE_BUSY; >> if (!dma_map_sg_attrs(dev->dev, iod->sg, iod->nents, dma_dir, >> - DMA_ATTR_NO_WARN)) >> + &attrs)) >> goto out; >> >> if (!nvme_setup_prps(dev, req, blk_rq_bytes(req))) >> -- >> 2.7.4 >> >> >> -- >> kernel-team mailing list >> kernel-team@lists.ubuntu.com >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Fri, Nov 18, 2016 at 06:58:08AM -0700, Tim Gardner wrote: > On 11/18/2016 06:36 AM, Seth Forshee wrote: > > On Wed, Nov 16, 2016 at 07:35:56AM -0700, Tim Gardner wrote: > >> BugLink: http://bugs.launchpad.net/bugs/1637565 > >> > >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > >> --- > >> drivers/nvme/host/pci.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > >> index 5256448..3931ceb 100644 > >> --- a/drivers/nvme/host/pci.c > >> +++ b/drivers/nvme/host/pci.c > >> @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, > >> enum dma_data_direction dma_dir = rq_data_dir(req) ? > >> DMA_TO_DEVICE : DMA_FROM_DEVICE; > >> int ret = BLK_MQ_RQ_QUEUE_ERROR; > >> + DEFINE_DMA_ATTRS(attrs); > > > > You also need this: > > > > dma_set_attr(DMA_ATTR_NO_WARN, &attrs); > > > > DMA_ATTR_NO_WARN was not introduced until v4.9-rc1. I think map_sg() > expects something from the enumerated set 'enum dma_attr'. > > I did find an instance of its use in arch/powerpc/kernel/iommu.c where > DMA_ATTR_NO_WARN is being queried. I think that is an erroneous backport > on my part, but harmless. In that case you should just revert 7c50722ad76b1b90538912fb84e2c3e206fab327 (nvme: use the DMA_ATTR_NO_WARN attribute).
On 11/18/2016 07:09 AM, Seth Forshee wrote: > On Fri, Nov 18, 2016 at 06:58:08AM -0700, Tim Gardner wrote: >> On 11/18/2016 06:36 AM, Seth Forshee wrote: >>> On Wed, Nov 16, 2016 at 07:35:56AM -0700, Tim Gardner wrote: >>>> BugLink: http://bugs.launchpad.net/bugs/1637565 >>>> >>>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >>>> --- >>>> drivers/nvme/host/pci.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c >>>> index 5256448..3931ceb 100644 >>>> --- a/drivers/nvme/host/pci.c >>>> +++ b/drivers/nvme/host/pci.c >>>> @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, >>>> enum dma_data_direction dma_dir = rq_data_dir(req) ? >>>> DMA_TO_DEVICE : DMA_FROM_DEVICE; >>>> int ret = BLK_MQ_RQ_QUEUE_ERROR; >>>> + DEFINE_DMA_ATTRS(attrs); >>> >>> You also need this: >>> >>> dma_set_attr(DMA_ATTR_NO_WARN, &attrs); >>> >> >> DMA_ATTR_NO_WARN was not introduced until v4.9-rc1. I think map_sg() >> expects something from the enumerated set 'enum dma_attr'. >> >> I did find an instance of its use in arch/powerpc/kernel/iommu.c where >> DMA_ATTR_NO_WARN is being queried. I think that is an erroneous backport >> on my part, but harmless. > > In that case you should just revert > 7c50722ad76b1b90538912fb84e2c3e206fab327 (nvme: use the DMA_ATTR_NO_WARN > attribute). > But that'll break powerpc. I'm inclined to leave it alone since it really is harmless. Or address it in another bug. rtg
On Fri, Nov 18, 2016 at 07:25:54AM -0700, Tim Gardner wrote: > On 11/18/2016 07:09 AM, Seth Forshee wrote: > > On Fri, Nov 18, 2016 at 06:58:08AM -0700, Tim Gardner wrote: > >> On 11/18/2016 06:36 AM, Seth Forshee wrote: > >>> On Wed, Nov 16, 2016 at 07:35:56AM -0700, Tim Gardner wrote: > >>>> BugLink: http://bugs.launchpad.net/bugs/1637565 > >>>> > >>>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > >>>> --- > >>>> drivers/nvme/host/pci.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > >>>> index 5256448..3931ceb 100644 > >>>> --- a/drivers/nvme/host/pci.c > >>>> +++ b/drivers/nvme/host/pci.c > >>>> @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, > >>>> enum dma_data_direction dma_dir = rq_data_dir(req) ? > >>>> DMA_TO_DEVICE : DMA_FROM_DEVICE; > >>>> int ret = BLK_MQ_RQ_QUEUE_ERROR; > >>>> + DEFINE_DMA_ATTRS(attrs); > >>> > >>> You also need this: > >>> > >>> dma_set_attr(DMA_ATTR_NO_WARN, &attrs); > >>> > >> > >> DMA_ATTR_NO_WARN was not introduced until v4.9-rc1. I think map_sg() > >> expects something from the enumerated set 'enum dma_attr'. > >> > >> I did find an instance of its use in arch/powerpc/kernel/iommu.c where > >> DMA_ATTR_NO_WARN is being queried. I think that is an erroneous backport > >> on my part, but harmless. > > > > In that case you should just revert > > 7c50722ad76b1b90538912fb84e2c3e206fab327 (nvme: use the DMA_ATTR_NO_WARN > > attribute). > > > > But that'll break powerpc. I'm inclined to leave it alone since it > really is harmless. Or address it in another bug. All 7c50722ad76b does is change the call to dma_map_sg() to dma_map_sg_attrs(), for the sole purpose of passing DMA_ATTR_NO_WARN. That backport is erroneous though because in xenial dma_map_sg_attrs() needs a pointer to the flags, not an int containing the flags. So it's broken _now_, because when ppc_iommu_map_sg() calls dma_get_attr() it's expecting a pointer, and we've given it something else. I only see two possibilities. Either we want to pass DMA_ATTR_NO_WARN, and we need to define the attrs, set DMA_ATTR_NO_WARN, and pass a pointer to the attrs. Or we don't want to pass DMA_ATTR_NO_WARN, so we can just call dma_map_sg() (which passes NULL for the attrs, and dma_get_attr() knows to check for that). It seems to me we want the first option. But you're right, it probably is material for a separate bug.
http://bugs.launchpad.net/bugs/1637565 Of course you are right about simply reverting 7c50722ad76b. I should start drinking coffee again. [PATCH 1/5] blk-mq: add blk_mq_alloc_request_hctx [PATCH 2/5] nvme.h: add NVMe over Fabrics definitions [PATCH 3/5] UBUNTU: [Config] CONFIG_NVME_VENDOR_EXT_GOOGLE=y [PATCH 4/5] UBUNTU: SAUCE: nvme: improve performance for virtual NVMe [PATCH 5/5] Revert "nvme: use the DMA_ATTR_NO_WARN attribute" rtg
On 18.11.2016 18:42, Tim Gardner wrote: > http://bugs.launchpad.net/bugs/1637565 > > Of course you are right about simply reverting 7c50722ad76b. I should start > drinking coffee again. > > [PATCH 1/5] blk-mq: add blk_mq_alloc_request_hctx > [PATCH 2/5] nvme.h: add NVMe over Fabrics definitions > [PATCH 3/5] UBUNTU: [Config] CONFIG_NVME_VENDOR_EXT_GOOGLE=y > [PATCH 4/5] UBUNTU: SAUCE: nvme: improve performance for virtual NVMe > [PATCH 5/5] Revert "nvme: use the DMA_ATTR_NO_WARN attribute" > > rtg > Possibly be more happy about this if the Xenial variant got some positive testing beforehand...
On 11/21/2016 03:31 AM, Stefan Bader wrote: > On 18.11.2016 18:42, Tim Gardner wrote: >> http://bugs.launchpad.net/bugs/1637565 >> >> Of course you are right about simply reverting 7c50722ad76b. I should start >> drinking coffee again. >> >> [PATCH 1/5] blk-mq: add blk_mq_alloc_request_hctx >> [PATCH 2/5] nvme.h: add NVMe over Fabrics definitions >> [PATCH 3/5] UBUNTU: [Config] CONFIG_NVME_VENDOR_EXT_GOOGLE=y >> [PATCH 4/5] UBUNTU: SAUCE: nvme: improve performance for virtual NVMe >> [PATCH 5/5] Revert "nvme: use the DMA_ATTR_NO_WARN attribute" >> >> rtg >> > > Possibly be more happy about this if the Xenial variant got some positive > testing beforehand... > > > I had an email from the tester indicating similar positive results as Yakkety. Quoting, "Also looking good! A quick run of fio benchmark 4k random reads shows us going from, in my one test case 23k IOPs to a much more palatable 180k IOPs." rtg
On 21.11.2016 16:28, Tim Gardner wrote: > On 11/21/2016 03:31 AM, Stefan Bader wrote: >> On 18.11.2016 18:42, Tim Gardner wrote: >>> http://bugs.launchpad.net/bugs/1637565 >>> >>> Of course you are right about simply reverting 7c50722ad76b. I should start >>> drinking coffee again. >>> >>> [PATCH 1/5] blk-mq: add blk_mq_alloc_request_hctx >>> [PATCH 2/5] nvme.h: add NVMe over Fabrics definitions >>> [PATCH 3/5] UBUNTU: [Config] CONFIG_NVME_VENDOR_EXT_GOOGLE=y >>> [PATCH 4/5] UBUNTU: SAUCE: nvme: improve performance for virtual NVMe >>> [PATCH 5/5] Revert "nvme: use the DMA_ATTR_NO_WARN attribute" >>> >>> rtg >>> >> >> Possibly be more happy about this if the Xenial variant got some positive >> testing beforehand... >> >> >> > > I had an email from the tester indicating similar positive results as > Yakkety. Quoting, "Also looking good! A quick run of fio benchmark 4k > random reads shows us going from, in my one test case 23k IOPs to a much > more palatable 180k IOPs." > Ah ok, not sure that info was in the bug report. Anyway, acked already.
On Fri, Nov 18, 2016 at 10:42:12AM -0700, Tim Gardner wrote: > http://bugs.launchpad.net/bugs/1637565 > > Of course you are right about simply reverting 7c50722ad76b. I should start > drinking coffee again. > > [PATCH 1/5] blk-mq: add blk_mq_alloc_request_hctx > [PATCH 2/5] nvme.h: add NVMe over Fabrics definitions > [PATCH 3/5] UBUNTU: [Config] CONFIG_NVME_VENDOR_EXT_GOOGLE=y > [PATCH 4/5] UBUNTU: SAUCE: nvme: improve performance for virtual NVMe > [PATCH 5/5] Revert "nvme: use the DMA_ATTR_NO_WARN attribute" All patches have been applied to xenial master-next *except* for the last one. The "nvme: use the DMA_ATTR_NO_WARN attribute" patch had already been reverted because it introduced a regression (bug #1644596). Cheers, -- Luís
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 5256448..3931ceb 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -623,6 +623,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, enum dma_data_direction dma_dir = rq_data_dir(req) ? DMA_TO_DEVICE : DMA_FROM_DEVICE; int ret = BLK_MQ_RQ_QUEUE_ERROR; + DEFINE_DMA_ATTRS(attrs); sg_init_table(iod->sg, req->nr_phys_segments); iod->nents = blk_rq_map_sg(q, req, iod->sg); @@ -631,7 +632,7 @@ static int nvme_map_data(struct nvme_dev *dev, struct request *req, ret = BLK_MQ_RQ_QUEUE_BUSY; if (!dma_map_sg_attrs(dev->dev, iod->sg, iod->nents, dma_dir, - DMA_ATTR_NO_WARN)) + &attrs)) goto out; if (!nvme_setup_prps(dev, req, blk_rq_bytes(req)))
BugLink: http://bugs.launchpad.net/bugs/1637565 Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- drivers/nvme/host/pci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)