Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255012 > unrolled thread

[PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:00 +0200
    Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:00 +0200
    [PATCH 1/7 v3] dma-mapping: add generic dma_get_page_shift API Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:00 +0200
    [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:00 +0200
      Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Alexey Kardashevskiy <aik@ozlabs.ru> - 2015-10-27 07:10 +0100
        Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift "Busch, Keith" <keith.busch@intel.com> - 2015-10-27 15:10 +0100
        Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-27 23:30 +0100
          Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Alexey Kardashevskiy <aik@ozlabs.ru> - 2015-10-28 02:10 +0100
            Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-28 03:00 +0100
              Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-10-28 03:30 +0100
                Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-28 03:40 +0100
                  Re: [PATCH 2/7 v2] powerpc/dma-mapping: override dma_get_page_shift Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-10-28 04:30 +0100
    [PATCH 4/7 v2] pseries/iommu: implement DDW-aware dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:00 +0200
      Re: [PATCH 4/7 v2] pseries/iommu: implement DDW-aware  dma_get_page_shift Alexey Kardashevskiy <aik@ozlabs.ru> - 2015-10-27 07:00 +0100
        Re: [PATCH 4/7 v2] pseries/iommu: implement DDW-aware  dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-27 23:30 +0100
    [RFC PATCH 6/7] sparc/dma-mapping: override dma_get_page_shift Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:10 +0200
    [PATCH 7/7 v2] drivers/nvme: default to the IOMMU page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:10 +0200
    [PATCH 5/7] [RFC PATCH 5/7] sparc: rename kernel/iommu_common.h ->  include/asm/iommu_common.h Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:10 +0200
      Re: [PATCH 5/7] [RFC PATCH 5/7] sparc: rename kernel/iommu_common.h  -> include/asm/iommu_common.h Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-23 23:10 +0200
    Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA David Miller <davem@davemloft.net> - 2015-10-27 02:20 +0100
      Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-27 23:30 +0100
        Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA "Busch, Keith" <keith.busch@intel.com> - 2015-10-27 23:40 +0100
          Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA David Miller <davem@davemloft.net> - 2015-10-28 01:40 +0100
            Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA "Busch, Keith" <keith.busch@intel.com> - 2015-10-28 15:00 +0100
              Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Christoph Hellwig <hch@infradead.org> - 2015-10-29 13:00 +0100
                Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-29 17:00 +0100
                  Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA "Busch, Keith" <keith.busch@intel.com> - 2015-10-29 18:30 +0100
                    [PATCH 1/1 v3] drivers/nvme: default to 4k device page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-30 22:40 +0100
                      Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size Keith Busch <keith.busch@intel.com> - 2015-10-30 22:50 +0100
                        Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-30 23:20 +0100
                      Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size Christoph Hellwig <hch@infradead.org> - 2015-11-03 14:20 +0100
                        Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size Keith Busch <keith.busch@intel.com> - 2015-11-03 14:50 +0100
                          [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-11-05 18:10 +0100
                            Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Christoph Hellwig <hch@infradead.org> - 2015-11-05 21:00 +0100
                              Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-11-05 23:00 +0100
                              Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-11-06 17:20 +0100
                              Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Christoph Hellwig <hch@infradead.org> - 2015-11-13 08:40 +0100
                                Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size Keith Busch <keith.busch@intel.com> - 2015-11-13 16:10 +0100
                  Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA David Miller <davem@davemloft.net> - 2015-10-30 02:40 +0100
                    Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-30 22:40 +0100
        Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Julian Calaby <julian.calaby@gmail.com> - 2015-10-28 00:00 +0100
          Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Julian Calaby <julian.calaby@gmail.com> - 2015-10-28 00:50 +0100
            Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2015-10-28 01:40 +0100
            Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA David Miller <davem@davemloft.net> - 2015-10-28 01:50 +0100
          Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-28 00:50 +0100
        Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA David Miller <davem@davemloft.net> - 2015-10-28 01:40 +0100
          Re: [PATCH 0/5 v3] Fix NVMe driver support on Power with 32-bit DMA Nishanth Aravamudan <nacc@linux.vnet.ibm.com> - 2015-10-28 03:00 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1257435

On 26.10.2015 [18:27:46 -0700], David Miller wrote:
> From: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> Date: Fri, 23 Oct 2015 13:54:20 -0700
> 
> > 1) add a generic dma_get_page_shift implementation that just returns
> > PAGE_SHIFT
> 
> I won't object to this patch series, but if I had implemented this I
> would have required the architectures to implement this explicitly,
> one-by-one.  I think it is less error prone and more likely to end
> up with all the architectures setting this correctly.

Well, looks like I should spin up a v4 anyways for the powerpc changes.
So, to make sure I understand your point, should I make the generic
dma_get_page_shift a compile-error kind of thing? It will only fail on
architectures that actually build the NVME driver (as the only caller).
But I'm not sure how exactly to achieve that, if you could give a bit
more detail I'd appreciate it!

Thanks,
Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257442

On Tue, Oct 27, 2015 at 03:20:10PM -0700, Nishanth Aravamudan wrote:
> On 26.10.2015 [18:27:46 -0700], David Miller wrote:
> > From: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > Date: Fri, 23 Oct 2015 13:54:20 -0700
> > 
> > > 1) add a generic dma_get_page_shift implementation that just returns
> > > PAGE_SHIFT
> > 
> > I won't object to this patch series, but if I had implemented this I
> > would have required the architectures to implement this explicitly,
> > one-by-one.  I think it is less error prone and more likely to end
> > up with all the architectures setting this correctly.
> 
> Well, looks like I should spin up a v4 anyways for the powerpc changes.
> So, to make sure I understand your point, should I make the generic
> dma_get_page_shift a compile-error kind of thing? It will only fail on
> architectures that actually build the NVME driver (as the only caller).
> But I'm not sure how exactly to achieve that, if you could give a bit
> more detail I'd appreciate it!

If you're suggesting to compile-time break architectures that currently
work just fine with NVMe, let me stop you right there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257512

From: "Busch, Keith" <keith.busch@intel.com>
Date: Tue, 27 Oct 2015 22:36:43 +0000

> If you're suggesting to compile-time break architectures that currently
> work just fine with NVMe, let me stop you right there.

Silently "working" without the architecture maintainer having to explicity
look at the new interface and make sure his platform is implementing it
properly is an extremely bad practice.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258051

On Tue, Oct 27, 2015 at 05:54:43PM -0700, David Miller wrote:
> From: "Busch, Keith" <keith.busch@intel.com>
> Date: Tue, 27 Oct 2015 22:36:43 +0000
> 
> > If you're suggesting to compile-time break architectures that currently
> > work just fine with NVMe, let me stop you right there.
> 
> Silently "working" without the architecture maintainer having to explicity
> look at the new interface and make sure his platform is implementing it
> properly is an extremely bad practice.

It won't work if it's wrong. It'll BUG_ON, and I'll be assigned to help
fix it, like what's happened here (on a private bugzilla).

The "new" interface for all the other architectures is the same as the
old one we've been using for the last 5 years.

I welcome x86 maintainer feedback to confirm virtual and DMA addresses
have the same offset at 4k alignment, but I have to insist we don't
break my currently working hardware to force their attention.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258742

On Wed, Oct 28, 2015 at 01:59:23PM +0000, Busch, Keith wrote:
> The "new" interface for all the other architectures is the same as the
> old one we've been using for the last 5 years.
> 
> I welcome x86 maintainer feedback to confirm virtual and DMA addresses
> have the same offset at 4k alignment, but I have to insist we don't
> break my currently working hardware to force their attention.

We had a quick cht about this issue and I think we simply should
default to a NVMe controler page size of 4k everywhere as that's the
safe default.  This is also what we do for RDMA Memory reigstrations and
it works fine there for SRP and iSER.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258880

On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
> On Wed, Oct 28, 2015 at 01:59:23PM +0000, Busch, Keith wrote:
> > The "new" interface for all the other architectures is the same as the
> > old one we've been using for the last 5 years.
> > 
> > I welcome x86 maintainer feedback to confirm virtual and DMA addresses
> > have the same offset at 4k alignment, but I have to insist we don't
> > break my currently working hardware to force their attention.
> 
> We had a quick cht about this issue and I think we simply should
> default to a NVMe controler page size of 4k everywhere as that's the
> safe default.  This is also what we do for RDMA Memory reigstrations and
> it works fine there for SRP and iSER.

So, would that imply changing just the NVMe driver code rather than
adding the dma_page_shift API at all? What about
architectures that can support the larger page sizes? There is an
implied performance impact, at least, of shifting the IO size down.

Sorry for the continuing questions -- I got lots of conflicting feedback
on the last series and want to make sure v4 is more acceptable.

Thanks,
Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1258928

On Thu, Oct 29, 2015 at 08:57:01AM -0700, Nishanth Aravamudan wrote:
> On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
> > We had a quick cht about this issue and I think we simply should
> > default to a NVMe controler page size of 4k everywhere as that's the
> > safe default.  This is also what we do for RDMA Memory reigstrations and
> > it works fine there for SRP and iSER.
> 
> So, would that imply changing just the NVMe driver code rather than
> adding the dma_page_shift API at all? What about
> architectures that can support the larger page sizes? There is an
> implied performance impact, at least, of shifting the IO size down.

It is the safe option, but you're right that it might have a measurable
performance impact (can you run an experiment?). Maybe we should just
change the driver to always use MPSMIN for the moment in the interest
of time, and you can flush out the new API before the next merge window.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259804 — [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

On 29.10.2015 [17:20:43 +0000], Busch, Keith wrote:
> On Thu, Oct 29, 2015 at 08:57:01AM -0700, Nishanth Aravamudan wrote:
> > On 29.10.2015 [04:55:36 -0700], Christoph Hellwig wrote:
> > > We had a quick cht about this issue and I think we simply should
> > > default to a NVMe controler page size of 4k everywhere as that's the
> > > safe default.  This is also what we do for RDMA Memory reigstrations and
> > > it works fine there for SRP and iSER.
> > 
> > So, would that imply changing just the NVMe driver code rather than
> > adding the dma_page_shift API at all? What about
> > architectures that can support the larger page sizes? There is an
> > implied performance impact, at least, of shifting the IO size down.
> 
> It is the safe option, but you're right that it might have a
> measurable performance impact (can you run an experiment?). Maybe we
> should just change the driver to always use MPSMIN for the moment in
> the interest of time, and you can flush out the new API before the
> next merge window.

Given that it's 4K just about everywhere by default (and sort of
implicitly expected to be, I guess), I think I'd prefer we default to
4K. That should mitigate the performance impact (I'll ask our IO team to
do some runs, but since this impacts functionality on some hardware, I
don't think it's too relevant for now). Unless there are NVMe devcies
with a MPSMAX < 4K? 

Something like the following?



We received a bug report recently when DDW (64-bit direct DMA on Power)
is not enabled for NVMe devices. In that case, we fall back to 32-bit
DMA via the IOMMU, which is always done via 4K TCEs (Translation Control
Entries).

The NVMe device driver, though, assumes that the DMA alignment for the
PRP entries will match the device's page size, and that the DMA aligment
matches the kernel's page aligment. On Power, the the IOMMU page size,
as mentioned above, can be 4K, while the device can have a page size of
8K, while the kernel has a page size of 64K. This eventually trips the
BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple
of 4K but not 8K (e.g., 0xF000).

In this particular case of page sizes, we clearly want to use the
IOMMU's page size in the driver. And generally, the NVMe driver in this
function should be using the IOMMU's page size for the default device
page size, rather than the kernel's page size. There is not currently an
API to obtain the IOMMU's page size across all architectures and in the
interest of a stop-gap fix to this functional issue, default the NVMe
device page size to 4K, with the intent of adding such an API and
implementation across all architectures in the next merge window.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
v1 -> v2:
  Based upon feedback from Christoph Hellwig, implement the IOMMU page
  size lookup as a generic DMA API, rather than an architecture-specific
  hack.

v2 -> v3:
  In the interest of fixing the functional problem in the short-term,
  just force the device page size to 4K and work on adding the new API
  in the next merge window.

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index ccc0c1f93daa..a9a5285bdb39 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	u32 aqa;
 	u64 cap = readq(&dev->bar->cap);
 	struct nvme_queue *nvmeq;
-	unsigned page_shift = PAGE_SHIFT;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
 	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
 




-Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259811 — Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
> Given that it's 4K just about everywhere by default (and sort of
> implicitly expected to be, I guess), I think I'd prefer we default to
> 4K. That should mitigate the performance impact (I'll ask our IO team to
> do some runs, but since this impacts functionality on some hardware, I
> don't think it's too relevant for now). Unless there are NVMe devcies
> with a MPSMAX < 4K? 

Right, I assumed MPSMIN was always 4k for the same reason you mentioned,
but you can hard code it like you've done in your patch.

The spec defines MPSMAX such that it's impossible to find a device
with MPSMAX < 4k.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259817 — Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

On 30.10.2015 [21:48:48 +0000], Keith Busch wrote:
> On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
> > Given that it's 4K just about everywhere by default (and sort of
> > implicitly expected to be, I guess), I think I'd prefer we default to
> > 4K. That should mitigate the performance impact (I'll ask our IO team to
> > do some runs, but since this impacts functionality on some hardware, I
> > don't think it's too relevant for now). Unless there are NVMe devcies
> > with a MPSMAX < 4K? 
> 
> Right, I assumed MPSMIN was always 4k for the same reason you mentioned,
> but you can hard code it like you've done in your patch.
> 
> The spec defines MPSMAX such that it's impossible to find a device
> with MPSMAX < 4k.

Great, thanks!

-Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261491 — Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index ccc0c1f93daa..a9a5285bdb39 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
>  	u32 aqa;
>  	u64 cap = readq(&dev->bar->cap);
>  	struct nvme_queue *nvmeq;
> -	unsigned page_shift = PAGE_SHIFT;
> +	/*
> +	 * default to a 4K page size, with the intention to update this
> +	 * path in the future to accomodate architectures with differing
> +	 * kernel and IO page sizes.
> +	 */
> +	unsigned page_shift = 12;
>  	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
>  	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;

Looks good as a start.  Note that all the MPSMIN/MAX checking could
be removed as NVMe devices must support 4k pages.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261508 — Re: [PATCH 1/1 v3] drivers/nvme: default to 4k device page size

On Tue, Nov 03, 2015 at 05:18:24AM -0800, Christoph Hellwig wrote:
> On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
> > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> > index ccc0c1f93daa..a9a5285bdb39 100644
> > --- a/drivers/block/nvme-core.c
> > +++ b/drivers/block/nvme-core.c
> > @@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
> >  	u32 aqa;
> >  	u64 cap = readq(&dev->bar->cap);
> >  	struct nvme_queue *nvmeq;
> > -	unsigned page_shift = PAGE_SHIFT;
> > +	/*
> > +	 * default to a 4K page size, with the intention to update this
> > +	 * path in the future to accomodate architectures with differing
> > +	 * kernel and IO page sizes.
> > +	 */
> > +	unsigned page_shift = 12;
> >  	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
> >  	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
> 
> Looks good as a start.  Note that all the MPSMIN/MAX checking could
> be removed as NVMe devices must support 4k pages.

MAX can go, and while it's probably the case that all devices support 4k,
it's not a spec requirement, so we should keep the dev_page_min check.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263436 — [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

On 03.11.2015 [13:46:25 +0000], Keith Busch wrote:
> On Tue, Nov 03, 2015 at 05:18:24AM -0800, Christoph Hellwig wrote:
> > On Fri, Oct 30, 2015 at 02:35:11PM -0700, Nishanth Aravamudan wrote:
> > > diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> > > index ccc0c1f93daa..a9a5285bdb39 100644
> > > --- a/drivers/block/nvme-core.c
> > > +++ b/drivers/block/nvme-core.c
> > > @@ -1717,7 +1717,12 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
> > >  	u32 aqa;
> > >  	u64 cap = readq(&dev->bar->cap);
> > >  	struct nvme_queue *nvmeq;
> > > -	unsigned page_shift = PAGE_SHIFT;
> > > +	/*
> > > +	 * default to a 4K page size, with the intention to update this
> > > +	 * path in the future to accomodate architectures with differing
> > > +	 * kernel and IO page sizes.
> > > +	 */
> > > +	unsigned page_shift = 12;
> > >  	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
> > >  	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
> > 
> > Looks good as a start.  Note that all the MPSMIN/MAX checking could
> > be removed as NVMe devices must support 4k pages.
> 
> MAX can go, and while it's probably the case that all devices support 4k,
> it's not a spec requirement, so we should keep the dev_page_min check.

Ok, here's an updated patch.

We received a bug report recently when DDW (64-bit direct DMA on Power)
is not enabled for NVMe devices. In that case, we fall back to 32-bit
DMA via the IOMMU, which is always done via 4K TCEs (Translation Control
Entries).

The NVMe device driver, though, assumes that the DMA alignment for the
PRP entries will match the device's page size, and that the DMA aligment
matches the kernel's page aligment. On Power, the the IOMMU page size,
as mentioned above, can be 4K, while the device can have a page size of
8K, while the kernel has a page size of 64K. This eventually trips the
BUG_ON in nvme_setup_prps(), as we have a 'dma_len' that is a multiple
of 4K but not 8K (e.g., 0xF000).

In this particular case of page sizes, we clearly want to use the
IOMMU's page size in the driver. And generally, the NVMe driver in this
function should be using the IOMMU's page size for the default device
page size, rather than the kernel's page size. There is not currently an
API to obtain the IOMMU's page size across all architectures and in the
interest of a stop-gap fix to this functional issue, default the NVMe
device page size to 4K, with the intent of adding such an API and
implementation across all architectures in the next merge window.

With the functionally equivalent v3 of this patch, our hardware test
exerciser survives when using 32-bit DMA; without the patch, the kernel
will BUG within a few minutes.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
v1 -> v2:
  Based upon feedback from Christoph Hellwig, implement the IOMMU page
  size lookup as a generic DMA API, rather than an architecture-specific
  hack.

v2 -> v3:
  In the interest of fixing the functional problem in the short-term,
  just force the device page size to 4K and work on adding the new API
  in the next merge window.

v3 -> v4:
  Rebase to the 4.3, including the new code locations.
  Based upon feedback from Keith Busch and Christoph Hellwig, remove the
  device max check, as the spec requires MPSMAX >= 4K.

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index e878590e71b6..00ca45bb0bc0 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1701,9 +1701,13 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 	u32 aqa;
 	u64 cap = readq(&dev->bar->cap);
 	struct nvme_queue *nvmeq;
-	unsigned page_shift = PAGE_SHIFT;
+	/*
+	 * default to a 4K page size, with the intention to update this
+	 * path in the future to accomodate architectures with differing
+	 * kernel and IO page sizes.
+	 */
+	unsigned page_shift = 12;
 	unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12;
-	unsigned dev_page_max = NVME_CAP_MPSMAX(cap) + 12;
 
 	if (page_shift < dev_page_min) {
 		dev_err(dev->dev,
@@ -1712,13 +1716,6 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 				1 << page_shift);
 		return -ENODEV;
 	}
-	if (page_shift > dev_page_max) {
-		dev_info(dev->dev,
-				"Device maximum page size (%u) smaller than "
-				"host (%u); enabling work-around\n",
-				1 << dev_page_max, 1 << page_shift);
-		page_shift = dev_page_max;
-	}
 
 	dev->subsystem = readl(&dev->bar->vs) >= NVME_VS(1, 1) ?
 						NVME_CAP_NSSRC(cap) : 0;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263520 — Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Looks fine,

Reviewed-by: Christoph Hellwig <hch@lst.de>

... but I doubt we'll ever bother updating it.  Most architectures
with arger page sizes also have iommus and would need different settings
for different iommus vs direct mapping for very little gain.  There's a
reason why we never bothered for RDMA either.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263569 — Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

On 05.11.2015 [11:58:39 -0800], Christoph Hellwig wrote:
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> ... but I doubt we'll ever bother updating it.  Most architectures
> with arger page sizes also have iommus and would need different settings
> for different iommus vs direct mapping for very little gain.  There's a
> reason why we never bothered for RDMA either.

Fair enough :) Thanks for all your reviews and comments.

-Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264135 — Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

On 05.11.2015 [11:58:39 -0800], Christoph Hellwig wrote:
> Looks fine,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> ... but I doubt we'll ever bother updating it.  Most architectures
> with arger page sizes also have iommus and would need different settings
> for different iommus vs direct mapping for very little gain.  There's a
> reason why we never bothered for RDMA either.

FWIW, whose tree should this go through? The bug only appears on Power,
afaik, but the patch is now just an NVMe change.

Thanks,
Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1268665 — Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

Jens, Keith: any chance to get this to Linux for 4.4 (and -stable)?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1269010 — Re: [PATCH 1/1 v4] drivers/nvme: default to 4k device page size

On Thu, Nov 12, 2015 at 11:37:54PM -0800, Christoph Hellwig wrote:
> Jens, Keith: any chance to get this to Linux for 4.4 (and -stable)?

I agreed, looks good to me.

Acked-by: Keith Busch <keith.busch@intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259161

From: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
Date: Thu, 29 Oct 2015 08:57:01 -0700

> So, would that imply changing just the NVMe driver code rather than
> adding the dma_page_shift API at all? What about
> architectures that can support the larger page sizes? There is an
> implied performance impact, at least, of shifting the IO size down.
> 
> Sorry for the continuing questions -- I got lots of conflicting feedback
> on the last series and want to make sure v4 is more acceptable.

In the long term I would be very happy to see us having a real interface
for this stuff, just my opinion...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259807

On 29.10.2015 [18:49:55 -0700], David Miller wrote:
> From: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> Date: Thu, 29 Oct 2015 08:57:01 -0700
> 
> > So, would that imply changing just the NVMe driver code rather than
> > adding the dma_page_shift API at all? What about
> > architectures that can support the larger page sizes? There is an
> > implied performance impact, at least, of shifting the IO size down.
> > 
> > Sorry for the continuing questions -- I got lots of conflicting feedback
> > on the last series and want to make sure v4 is more acceptable.
> 
> In the long term I would be very happy to see us having a real interface
> for this stuff, just my opinion...

Yep, I think I'll try and balance the two -- fix NVMe for now with a 4K
page size as suggested by Christoph, and then work on the more complete
API for the next merge.

Thanks,
Nish

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web