nvme: use __GFP_NOWARN for iod allocation

This begs the question is we should limit the size in general. The command overhead is low enough that I think we should default to something sane that doesn't require _any_ > 0 order allocations.

Our default 1280kb will require 10248 bytes of iod, which is an order 2 allocation.... That's not helping tail latencies in data centers, where memory is always full and fragmented.

Quoted 7 lines · Jens Axboe

This begs the question is we should limit the size in general. The command overhead is low enough that I think we should default to something sane that doesn't require _any_ > 0 order allocations.

Our default 1280kb will require 10248 bytes of iod, which is an order 2 allocation.... That's not helping tail latencies in data centers, where memory is always full and fragmented.

In terms of sizing, defaulting to 256kb might not be a bad idea. That'd be 2056 bytes for a normal config. Alternatively we could go 1280/4 == 320k, which would be 2568 bytes.

In any case, I think it's something that's worth doing.

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

This begs the question is we should limit the size in general. The command overhead is low enough that I think we should default to something sane that doesn't require _any_ > 0 order allocations.

Our default 1280kb will require 10248 bytes of iod, which is an order 2 allocation.... That's not helping tail latencies in data centers, where memory is always full and fragmented.

Quoted 5 lines

In terms of sizing, defaulting to 256kb might not be a bad idea. That'd be 2056 bytes for a normal config. Alternatively we could go 1280/4 == 320k, which would be 2568 bytes.

In any case, I think it's something that's worth doing.

Talking more to myself - higher order allocations needed to make progress isn't a viable approach. There may _never_ be any available, which means we are stuck since this isn't backed by a mempool.

How about something like the below. Default to a size that will never require > PAGE_SIZE allocation, and back that with a single entry mempool. Even if we don't wait on it, if we keep retrying we'll always be able to allocate memory and make forward progress. And allocating a single page is a hell of a lot more likely than a 2nd order allocation.

Quoted 20 lines · Jens Axboe

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

This begs the question is we should limit the size in general. The command overhead is low enough that I think we should default to something sane that doesn't require _any_ > 0 order allocations.

Our default 1280kb will require 10248 bytes of iod, which is an order 2 allocation.... That's not helping tail latencies in data centers, where memory is always full and fragmented.

Quoted 5 lines

In terms of sizing, defaulting to 256kb might not be a bad idea. That'd be 2056 bytes for a normal config. Alternatively we could go 1280/4 == 320k, which would be 2568 bytes.

In any case, I think it's something that's worth doing.

Talking more to myself - higher order allocations needed to make progress isn't a viable approach. There may _never_ be any available, which means we are stuck since this isn't backed by a mempool.

Quoted 6 lines

How about something like the below. Default to a size that will never require > PAGE_SIZE allocation, and back that with a single entry mempool. Even if we don't wait on it, if we keep retrying we'll always be able to allocate memory and make forward progress. And allocating a single page is a hell of a lot more likely than a 2nd order allocation.

If we go this route, there is a great deal of cleanup that can follow on. For example, we'll never need to chain PRPs, so nvme_iod setup/free can become a lot simpler, and the struct iod npages and hidden "iod_list" member can go away.

I should also mention that this came out of a real life hang on a production box. I suppose we haven't seen a lot of these since a lot of nvme controllers have a hw limit of 128k, but the ones that don't can pretty easily hit the hang of needing 2^2 pages and failing miserably forever.

Quoted 5 lines · Jens Axboe

I should also mention that this came out of a real life hang on a production box. I suppose we haven't seen a lot of these since a lot of nvme controllers have a hw limit of 128k, but the ones that don't can pretty easily hit the hang of needing 2^2 pages and failing miserably forever.

I can certainly believe that. I'm totally fine with setting a smaller upper bound on the IO size. There really isn't a meaningful bandwidth advantage to larger transfers on NVMe PCIe once you get over 256k anyway.

Since you're making the change to core, let's wait to hear if there are any concerns on the fabrics front.

If the fabrics folks have concerns, we could make it dependent on it being a pci device. That's the case I care about anyway, and the one that's currently breaking for me.

Quoted 3 lines · Jens Axboe

If the fabrics folks have concerns, we could make it dependent on it being a pci device. That's the case I care about anyway, and the one that's currently breaking for me.

Fabrics uses the SG chain allocator that SCSI also uses, so it should already be fine, including for larger I/O sizes.

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Jens Axboe

If the fabrics folks have concerns, we could make it dependent on it being a pci device. That's the case I care about anyway, and the one that's currently breaking for me.

Fabrics uses the SG chain allocator that SCSI also uses, so it should already be fine, including for larger I/O sizes.

I'll change it to ignore it for fabrics. What's the best way to check? ctrl->sqsize?

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Jens Axboe

If the fabrics folks have concerns, we could make it dependent on it being a pci device. That's the case I care about anyway, and the one that's currently breaking for me.

Fabrics uses the SG chain allocator that SCSI also uses, so it should already be fine, including for larger I/O sizes.

Quoted 2 lines

I'll change it to ignore it for fabrics. What's the best way to check? ctrl->sqsize?

ctrl->ops->flags & NVME_F_FABRICS

but in the end we probably want a separate flag, as that is an implementation detail, and not because these are fabrics drivers.

I'll be spending the rest of the day on phone calls, so it'll take some time to do more on this than writing some email, but I'll try to find some time to look into these issues.

Quoted 10 lines · Christoph Hellwig

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Christoph Hellwig

Quoted 3 lines · Jens Axboe

If the fabrics folks have concerns, we could make it dependent on it being a pci device. That's the case I care about anyway, and the one that's currently breaking for me.

Fabrics uses the SG chain allocator that SCSI also uses, so it should already be fine, including for larger I/O sizes.

Quoted 7 lines

Quoted 2 lines

I'll change it to ignore it for fabrics. What's the best way to check? ctrl->sqsize?

ctrl->ops->flags & NVME_F_FABRICS

but in the end we probably want a separate flag, as that is an implementation detail, and not because these are fabrics drivers.

Probably fine for now with a comment, since it is pretty tightly coupled to the fact that we prefer larger commands on fabrics, and don't care so much about that for a local controller.

For now the check is moot though, since we are doing the same allocations...

+#define NVME_MAX_SEGS 127

I guess we can live with it for now with a big comment

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2384

Quoted 8 lines

drivers/nvme/host/core.c +3 -1

 	ctrl->cntlid = le16_to_cpup(&id->cntlid);
 	if (id->mdts)
 		max_hw_sectors = 1 << (id->mdts + page_shift - 9);
-	else
+	else if (ctrl->ops->flags & NVME_F_FABRICS)
 		max_hw_sectors = UINT_MAX;
+	else
+		max_hw_sectors = NVME_MAX_KB_SZ << 1;

Can this I think is better done by setting ctrl->max_hw_sectors in the PCIe driver (similar to what the other drivers already do) and then rely on the min_not_zero below to factor in MDTS.

drivers/nvme/host/nvme.h extern unsigned int admin_timeout; · line 40

Quoted 5 lines

drivers/nvme/host/nvme.h +5 -0

+/*
+ * These can be higher, but we need to ensure that any command doesn't
+ * require an sg allocation that needs more than a page of data.
+ */
+#define NVME_MAX_KB_SZ	4096

I think this should move the pci.c

Quoted 1 line

+#define NVME_MAX_SEGS 127

And this could use a good explanation. Especially as I think that we shouldn't apply the limit for fabrics. So instead we might just add a new ctrl->max_segments field, which PCI sets, and which then is min() with the otherwise calculated value.

Quoted 3 lines

I'll be spending the rest of the day on phone calls, so it'll take some time to do more on this than writing some email, but I'll try to find some time to look into these issues.

OK sounds good, would be nice to get this hashed out as I think some variant of this should go into 4.18.

Quoted 6 lines · Jens Axboe

Quoted 3 lines

I'll be spending the rest of the day on phone calls, so it'll take some time to do more on this than writing some email, but I'll try to find some time to look into these issues.

OK sounds good, would be nice to get this hashed out as I think some variant of this should go into 4.18.

Fine with me, but I'd rather not rush it out this week to give us a little more time for details and aim for merging it to 4.18 next week.

Oh, I'm totally fine with that, no rush outside of just wanting to ensure it ends up in 4.18. But there's plenty of time for that.

Below is cleaner I think, keeping it all in pci.c and fabrics should just work. I'll test...

Quoted 5 lines · Jens Axboe

Oh, I'm totally fine with that, no rush outside of just wanting to ensure it ends up in 4.18. But there's plenty of time for that.

Below is cleaner I think, keeping it all in pci.c and fabrics should just work. I'll test...

Ok, this actually looks harmless enough that with successfull testing and an ack from Keith we could queue it up this week..

From: Jens Axboe <axboe at kernel.dk>
Subject: [PATCH] nvme: limit max IO size and segments for pci-e

nvme requires an sg table allocation for each request. If the request is large, then the allocation can become quite large. For instance, with our default software settings of 1280KB IO size, we'll need 10248 bytes of sg table. That turns into a 2nd order allocation, which we can't always guarantee. If we fail the allocation, blk-mq will retry it later. But there's no guarantee that we'll EVER be able to allocate that much contigious memory.

Limit the IO size such that we never need more than a single page of memory. That's a lot faster and more reliable. Then back that allocation with a mempool, so that we know we'll always be able to succeed the allocation at some point.

Signed-off-by Jens Axboe <axboe at kernel.dk>

Quoted 16 lines · Jens Axboe

From: Jens Axboe <axboe at kernel.dk>
Subject: [PATCH] nvme: limit max IO size and segments for pci-e

nvme requires an sg table allocation for each request. If the request is large, then the allocation can become quite large. For instance, with our default software settings of 1280KB IO size, we'll need 10248 bytes of sg table. That turns into a 2nd order allocation, which we can't always guarantee. If we fail the allocation, blk-mq will retry it later. But there's no guarantee that we'll EVER be able to allocate that much contigious memory.

Limit the IO size such that we never need more than a single page of memory. That's a lot faster and more reliable. Then back that allocation with a mempool, so that we know we'll always be able to succeed the allocation at some point.

Quoted 1 line

Signed-off-by Jens Axboe <axboe at kernel.dk>

This looks good to me. Thanks!

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

Thanks Keith - Christoph/Keith, where do you want to queue this up? I can do it directly, or we can do it through the nvme tree.

Quoted 2 lines · Jens Axboe

Thanks Keith - Christoph/Keith, where do you want to queue this up? I can do it directly, or we can do it through the nvme tree.

I've queued it up in the nvme tree already.

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

This begs the question is we should limit the size in general. The command overhead is low enough that I think we should default to something sane that doesn't require _any_ > 0 order allocations.

Our default 1280kb will require 10248 bytes of iod, which is an order 2 allocation.... That's not helping tail latencies in data centers, where memory is always full and fragmented.

Quoted 5 lines

In terms of sizing, defaulting to 256kb might not be a bad idea. That'd be 2056 bytes for a normal config. Alternatively we could go 1280/4 == 320k, which would be 2568 bytes.

In any case, I think it's something that's worth doing.

Talking more to myself - higher order allocations needed to make progress isn't a viable approach. There may _never_ be any available, which means we are stuck since this isn't backed by a mempool.

How about something like the below. Default to a size that will never require > PAGE_SIZE allocation, and back that with a single entry mempool. Even if we don't wait on it, if we keep retrying we'll always be able to allocate memory and make forward progress. And allocating a single page is a hell of a lot more likely than a 2nd order allocation.

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2382

Quoted 3 lines

drivers/nvme/host/core.c +1 -1

 	else
-		max_hw_sectors = UINT_MAX;
+		max_hw_sectors = NVME_MAX_KB_SZ << 1;

I think this only makes sense when using PRPs. If we use SGLs in Fabrics, or in PCIe controllers that support them we are only bound by the number of SGL segments and MDTS.

This also seems to miss handling the case where MDTS is set, but above our software limit.

Quoted 7 lines · Christoph Hellwig

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2382

Quoted 3 lines

drivers/nvme/host/core.c +1 -1

 	else
-		max_hw_sectors = UINT_MAX;
+		max_hw_sectors = NVME_MAX_KB_SZ << 1;

I think this only makes sense when using PRPs. If we use SGLs in Fabrics, or in PCIe controllers that support them we are only bound by the number of SGL segments and MDTS.

The problem is that we have to limit the max size, since the sg allocation depends on that, marginally. In reality, the segments will dictate the max size anyway.

Quoted 3 lines

This also seems to miss handling the case where MDTS is set, but above our software limit.

Shouldn't, we're using the min of MDTS and the software limit.

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Christoph Hellwig

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2382

Quoted 3 lines

drivers/nvme/host/core.c +1 -1

 	else
-		max_hw_sectors = UINT_MAX;
+		max_hw_sectors = NVME_MAX_KB_SZ << 1;

I think this only makes sense when using PRPs. If we use SGLs in Fabrics, or in PCIe controllers that support them we are only bound by the number of SGL segments and MDTS.

Quoted 3 lines

The problem is that we have to limit the max size, since the sg allocation depends on that, marginally. In reality, the segments will dictate the max size anyway.

The sg allocation depends on the number of segments. The PRP allocation (if using PRPs) depends purely on the size + a little slack for unaligned first or last arguments.

Talking more to myself - higher order allocations needed to make progress isn't a viable approach. There may _never_ be any available, which means we are stuck since this isn't backed by a mempool.

How about something like the below. Default to a size that will never require > PAGE_SIZE allocation, and back that with a single entry mempool. Even if we don't wait on it, if we keep retrying we'll always be able to allocate memory and make forward progress. And allocating a single page is a hell of a lot more likely than a 2nd order allocation.

Quoted 3 lines · Jens Axboe

Talking more to myself - higher order allocations needed to make progress isn't a viable approach. There may _never_ be any available, which means we are stuck since this isn't backed by a mempool.

Yes, I've complained about that a few times in the past, but so far no one has hit it in practice.

Quoted 6 lines

How about something like the below. Default to a size that will never require > PAGE_SIZE allocation, and back that with a single entry mempool. Even if we don't wait on it, if we keep retrying we'll always be able to allocate memory and make forward progress. And allocating a single page is a hell of a lot more likely than a 2nd order allocation.

I suspect some people might be unhappy about the limit on I/O sizes. That is why we want through all that effort with chained S/G lists in SCSI, which are now also used by NVMe over Fabrics. But except for that we absolutely have to move to mempools.

That may be the case, but it's a lot less of an issue than SCSI is, since the command overhead is suitably small. So I don't see a big issue with limiting it.

Besides, having to do higher order allocations for an
IO is bound to pose much bigger issues than having two smaller IOs instead. That's especially true in production, where memory allocation stalls are pretty frequent.

Quoted 3 lines · Jens Axboe

That may be the case, but it's a lot less of an issue than SCSI is, since the command overhead is suitably small. So I don't see a big issue with limiting it.

Not if you are using NVMe over FC, which uses exactly the same protocol to transfer a 64 byte NVMe SQ vs a typically 16 byte SCSI CDB..

Quoted 4 lines

Besides, having to do higher order allocations for an
IO is bound to pose much bigger issues than having two smaller IOs instead. That's especially true in production, where memory allocation stalls are pretty frequent.

But the whole point of our chained sg allocator is that we don't do high order allocations, but multiple smaller ones and then chain them.

From a quick look, the fabric code goes through the same nvme_queue_rq()

Quoted 1 line

From a quick look, the fabric code goes through the same nvme_queue_rq()

and ends up doing the same iod->sg allocation. I don't see any use of chained sg lists, we're allocating one big sg table for the command.

Quoted 3 lines · Jens Axboe

Quoted 1 line

From a quick look, the fabric code goes through the same nvme_queue_rq()

and ends up doing the same iod->sg allocation. I don't see any use of chained sg lists, we're allocating one big sg table for the command.

nvme_queue_rq is only used for PCIe.

E.g. for RDMA the call is:

nvme_rdma_queue_rq
-> nvme_rdma_map_data
-> sg_alloc_table_chained

similar for FC and loop.