- Download Diff

Summary:

Disconnect NVMe submission and completion queues

Review Request #1979 — Created June 7, 2019 and submitted June 21, 2019, 8:33 a.m.

Information
Owner:	pwinder
Repository:	illumos-gate
Branch:
Bugs:
Depends On:
Reviewers
Groups:	general
People:	hans, rm

Description

Separate the submission and completion queues. Each queue pair has a dedicated submission queue, but may have shared completion queues. This allows the number of submission queues to be increased beyond the current limitation set by the nunber of interrupt vectors.

The change includes a few bug fixes we have come across. They are highlighted in the diff.

See: https://www.illumos.org/issues/11202

Issues also covered here are:
nvme may queue more submissions than allowed
nvme_get_logpage() can allocate a too small buffer to receive logpage data
Panic in nvme_fill_prp() because of miscalculation of the number of PRPs per page
nvme in polled mode ignores the command call back

Testing Done

The base of this code has been in extensive use in WDC's Intelliflash O/S.

Within Illumos (using SmartOS), it has been tested by running the full zfs test suite against a set of NVMe drives, as well as stress tested using vdbench.

It has been run on raw hardware with Dual Intel(R) Xeon(R) Gold 6130, with NVMe drives in a PCI fanout behind a Switchtec switch.

Also tested in a Fusion VM with a NVMe root disk.

Issues

0
0
20
0
All Issues: 20

Description	From	Last Updated
This should clarify which will win then, the lower or higher value.	rm	June 11, 2019, 2:43 p.m.
larger than	rm	June 11, 2019, 2:43 p.m.
Is it legal to hold these for multiple queues at any given time? If so what order do we need ...	rm	June 11, 2019, 2:43 p.m.
Could you perhaps add a short comment on how or why this works or what the algorithm is called?	hans	June 12, 2019, 12:33 p.m.
This will overflow a UINT32 to zero when we have the largest value here. We should make sure no one ...	rm	June 12, 2019, 12:34 p.m.
Can another bug be opened that describes this particular problem (it can be fixed in the same commit as the ...	rm	June 12, 2019, 1:05 p.m.
Is there a reason we don't pass ncq as a uint16_t given that's the valid range that we can actually ...	rm	June 12, 2019, 12:36 p.m.
If we're casting ncq, shouldn't we do that before we actually do the allocation? This also means that currently if ...	rm	June 12, 2019, 12:38 p.m.
Shouldn't this be a uint16_t to actually match the width of the n_cq_count? This value should never be negative.	rm	June 12, 2019, 12:39 p.m.
Can we add an ASSERT that the ncq mutex is held here please?	rm	June 11, 2019, 2:47 p.m.
Any reason this isn't unsigned? This value should never be negative.	rm	June 12, 2019, 1:05 p.m.
I'd strongly suggest that the size of the DMA region be set to zero, which will do the entire region. ...	rm	June 11, 2019, 3 p.m.
Doesn't this need to be cast to void if we're going to ignore the return value?	rm	June 11, 2019, 2:45 p.m.
I would change the size to zero here as well.	rm	June 11, 2019, 3 p.m.
Can we please open another bug that covers this and can be fixed in this change?	rm	June 11, 2019, 3:36 p.m.
Worth adding an ASSERT that neither of these are zero before we do the subtraction?	rm	June 11, 2019, 3:04 p.m.
Do we need to log this every time we attach a device?	rm	June 11, 2019, 3:02 p.m.
Should we actually change this to be MAX(n_submission_queues, and n_completion_queues) so that way if we need to change things in ...	rm	June 11, 2019, 3:09 p.m.
Worth opening another bug for this issue?	rm	June 11, 2019, 3:36 p.m.
Extra word in comment: "When the there are". Have gone through all the changes, look good to me.	yyang	June 18, 2019, 6:01 a.m.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

When len > 1 page, we could get 2 coookies

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

We spotted this - although never actually hit it.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)
```
The size argument was incorrect
```

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

These caused a panic when running zfs test suite

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

I wondered if it make sense to do something with io-queue-len if it is set and the new values are not present? It is possible that some people will have customised this in their driver conf file.

I then checked and it seems that any local modifications to nvme.conf are not preserved on upgrade; that seems wrong but means you do not need to worry about backwards compatibility here at least.

Ship it!

pwinder June 10, 2019, 8:31 a.m.

The algorithm effectively computes 2^(log2(n - 1) + 1).
The n-- takes care of the case when n is already a power-of-2.
The shift operations set all bits to 1 from the msb set to lsb.
I will add a comment on these lines to the code, and will publish an update when more have reviewed.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Could you perhaps add a short comment on how or why this works or what the algorithm is called?

pwinder June 7, 2019, 4:38 p.m.
```
Of course
```

Thanks for putting this together, I have a few questions and some minor feedback. In general, the approach seems good.

usr/src/man/man7d/nvme.7d (Diff revision 1)

Show all issues

This should clarify which will win then, the lower or higher value.

usr/src/man/man7d/nvme.7d (Diff revision 1)
Show all issues
```
larger than
```

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Is it legal to hold these for multiple queues at any given time? If so what order do we need to take these? For example, if we have more sq's than cq's, we should probably comment on how to lock everything.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

This will overflow a UINT32 to zero when we have the largest value here. We should make sure no one actually uses it with anything larger than 1 << 31. The most common, though hopefully unlikely to happen case would be someone passing UINT32_MAX as an attempt to have a value set to -1.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Can another bug be opened that describes this particular problem (it can be fixed in the same commit as the queue separation).

Why did we need to round up to a power of two to guarantee that we only get a single DMA cookie? If we only want a single DMA cookie, all we need to do is just ask for a single DMA cookie in the request. It's not clear why we need more memory to be allocated. With the sgllen set to one, that's the thing that actually guarantees we only get a single cookie.

Is this comment trying to help the system by rounding it up to a power of two to try and make that more likely? Is there a reason we're doing that to a power of two and not say page size? It seems like we could be wasting a lot of contiguous DMA memory, which if a device is hot plugged late in life of the system won't be available.

Given that we can use a PRP list to represent a queue (based on my read of the spec), we should probably look at coming back to that for future work so we can make this much more likely to work and not have to constrain ourselve to a single cookie -- though I will admit it's drastically simpler.

pwinder June 11, 2019, 8:42 a.m.

The work-around is for this particular check near the start of i_ddi_mem_alloc:
    if (attr->dma_attr_minxfer == 0 || attr->dma_attr_align == 0 ||
        (attr->dma_attr_align & (attr->dma_attr_align - 1)) ||
        (attr->dma_attr_minxfer & (attr->dma_attr_minxfer - 1))) {
                    return (DDI_FAILURE);
    }

So, if dma_attr_min and dma_attr_align (ie the len argument to nvme_zalloc_queue_dma()) are not a power of 2 the memory allocation fails.

pwinder June 11, 2019, 11:09 a.m.

Been giving this some more thought.
We should set the dma_attr_align to a fixed size (I'd suggest n_pagesize), and then the memory allocation should work and the dma_attr_sgllen should work as expected

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Is there a reason we don't pass ncq as a uint16_t given that's the valid range that we can actually create something? I know that we're casting it here, but it seems like we're there's never a case where it's correct for this to take a uint16_t and the callers should make sure that they are doing proper size checking before reaching here. I've left additional comments assuming you don't change this.

pwinder June 12, 2019, 12:40 p.m.

The cq array includes the admin completion queue as well as all the others. Which does mean the array can be UINT16_MAX + 1.
I have changed ncq to be uint_t.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

If we're casting ncq, shouldn't we do that before we actually do the allocation? This also means that currently if we get a value that's larger than a uint16_t, we'll allocate a different number of arrays than we have recorded, which means we'll incorrectly free this.

pwinder June 12, 2019, 12:40 p.m.

I have changed n_cq_count to uint_t.
There csn also be UINT16_MAX + 1 qpairs (1 admin + UINT16_MAX std). I have changed n_ioq_count to uint_t too.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Shouldn't this be a uint16_t to actually match the width of the n_cq_count? This value should never be negative.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Can we add an ASSERT that the ncq mutex is held here please?

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Any reason this isn't unsigned? This value should never be negative.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

I'd strongly suggest that the size of the DMA region be set to zero, which will do the entire region. This is what you're trying to do and will also cut out on a potential failure mode if that size is not right. In this case, casting to void means we could miss errors. I realize this is just a moved/renamed version of what already exists, but seems like we should fix that now.

pwinder June 11, 2019, 2:59 p.m.

Changed the call to use a size of zero.
99% of the kernel code either voids ddi_dma_sync() or simply prints a warning and continues. I have changed it to do the latter.

rm June 11, 2019, 3:03 p.m.

If we pass zero for the offset and length, then it's defined not to fail, which is why I was suggesting changing it.

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Doesn't this need to be cast to void if we're going to ignore the return value?

pwinder June 11, 2019, 2:44 p.m.
```
It is a void function.
```

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)
Show all issues
```
I would change the size to zero here as well.
```

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Can we please open another bug that covers this and can be fixed in this change?

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Worth adding an ASSERT that neither of these are zero before we do the subtraction?

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)
Show all issues
```
Do we need to log this every time we attach a device?
```
1. pwinder June 11, 2019, 3:04 p.m.
  removed

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)

Show all issues

Should we actually change this to be MAX(n_submission_queues, and n_completion_queues) so that way if we need to change things in the future around how many to have, we won't hit any implicit assumptions. Otherwise, should we assert or do something else to make sure that can't happen in the face of future refactoring?

pwinder June 11, 2019, 3:23 p.m.

I'll add an ASSERT(n_submission_queues >= n_completion_queues)

usr/src/uts/common/io/nvme/nvme.c (Diff revision 1)
Show all issues
```
Worth opening another bug for this issue?
```

Thanks for the feedback.
I'm reviewing the comments, I will open a few more bugs and address the comments aqnd re-post in a few days

Description:

		Separate the submission and completion queues. Each queue pair has a dedicated submission queue, but may have shared completion queues. This allows the number of submission queues to be increased beyond the current limitation set by the nunber of interrupt vectors.

		The change includes a few bug fixes we have come across. They are highlighted in the diff.

		See: https://www.illumos.org/issues/11202
	+
	+	Issues also covered here are:
	+	nvme may queue more submissions than allowed
	+	nvme_get_logpage() can allocate a too small buffer to receive logpage data
	+	Panic in nvme_fill_prp() because of miscalculation of the number of PRPs per page
	+	nvme in polled mode ignores the command call back

Change Summary:

Hopefully addresses Robert raised.

Diff:

Revision 2 (+451 -97)

Show changes

	usr/src/man/man7d/nvme.7d
	usr/src/uts/common/io/nvme/nvme.conf
	usr/src/uts/common/io/nvme/nvme.c
	usr/src/uts/common/io/nvme/nvme_var.h

Ship it!

```
Ship It!
```

usr/src/uts/common/io/nvme/nvme.c (Diff revision 2)

Show all issues

Extra word in comment: "When the there are".

Have gone through all the changes, look good to me.

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

Disconnect NVMe submission and completion queues

Description:

Change Summary:

Diff:

Status: Closed (submitted)