nvme: coherently read status

[imp]

Summary

When reading a completion record, avoid a race with the device. If the
host starts to read the completion record and then the device updates it
while we're reading it, we can have the early part of the record be old
and the later part of the record be new. This leads us to mistakenly
think that the record is in phase and we use the old values and look
at an already completed entry, which has no current tracker.

To work around this problem, we atomically read the status with acquire
semantics. If it's in phase, we then re-read the entire completion
record. In addition we resync the dmatag to reflect changes since the
prior loop for the bouncing dma case.

Found by: jrtc27 (this fix is based in part on her D30995 fix)
Sponsored by: Netflix

Test Plan

A slightly different approach to fix this race than in D30995. We should consider both

[jrtc27]

I don't think you can skip it on the first iteration:

Host                  Drive

                   Write A.CID
                        |
                        V
                  Write A.STATUS
                        |
                        V
               +--- Send IRQ
               |        |
Receive IRQ <--+        |
     |                  V
     |             Write B.CID
     |                  |
     |                  V
     |            Write B.STATUS
     |                  |
     V                  |
   Sync                 |
     |                  |
     V                  |
 Read A.CID             |
     |                  |
     V                  |
Read A.STATUS           |
     |                  |
     V                  |
  Process               |
     |                  |
     V                  |
Read B.CID              |
     |                  |
     V                  |
Read B.STATUS           |
     |                  |
     V                  |
  Process               |
     |                  V
     |         +--- Send IRQ
     V         |        |
Receive IRQ <--+        |
     |                  |
     V                  |
Read C.CID              |
     |                  V
     |             Write C.CID
     |                  |
     |                  V
     |            Write C.STATUS
     V
Read C.STATUS

[jrtc27]

Well, at least in the bouncing case, since the load acquire doesn't actually help you. I think in the coherent non-bouncing DMA case the load acquire does address the issue, but also the sync isn't needed at all there.

[chuck]

FWIW, the completion handler in Linux does something similar, but it orders the memory synchronization differently. Effectively, it does

while ( phase_matches() ) {
    DMA_read_memory_barrier();
    process_completion():
}

where the phase_matches() function reads status and masks the phase bit.

[imp]

In D31002#697523, @chuck wrote:

FWIW, the completion handler in Linux does something similar, but it orders the memory synchronization differently. Effectively, it does

while ( phase_matches() ) {
    DMA_read_memory_barrier();
    process_completion():
}

where the phase_matches() function reads status and masks the phase bit.

Yes. That's effectively the order that Jessica's patch has. And I agree with her there is no 'first' here, so I'll update this patch with that.

[imp]

fold in Jessica's observation that sync is need, as well as chuck's feedback.

[imp]

silly compiler error due to too many retypings of the same code :(.

[imp]

sys/dev/nvme/nvme_qpair.c

598

with the sync below (and above), do I need the atomic here?

[jrtc27]

sys/dev/nvme/nvme_qpair.c

592

cid not sqid, we don't look at the latter.

598

No, it can then just be a direct read of the field. I was hoping my version would be optimised by the compiler to be entirely equivalent, but it seems not... so I think reading just the field rather than copying the struct like I did is a good idea. I don't know why it didn't optimise it though, it really should have been able to (and the pass that does so is one I'm far too familiar with...).

607

I'd only check the phase. status is 2 bytes so the initial read can technically tear if you're not careful; at least it will currently on riscv due to the packed struct, i.e. the byte of status that doesn't include the phase bit could have been read before and inconsistent with the byte that does include the phase bit.

[chuck]

sys/dev/nvme/nvme_qpair.c

598

For a compliant device, the NVMe specification says:

If a completion queue entry is constructed via multiple writes, the Phase Tag bit shall be updated in the last write of that completion queue entry.

I think this implies that the sync's are sufficient and the atomic isn't needed.

[imp]

sys/dev/nvme/nvme_qpair.c

592

tweaked.

598

thanks guys. removed.

607

Gotcha. I'd prefer to over-test, but if it doesn't matter, then it's better to not, even though the result is a bit awkward.

[imp]

review comments: remove atomic, tweak wording, test phase.

[jrtc27]

sys/dev/nvme/nvme_qpair.c

607

Somehow I forgot to mention in my previous comment (but thought it, and in fact was what made me then think about the assertion itself) that the message for this assertion doesn't make sense to me?

[imp]

fix assert message.

[imp]

sys/dev/nvme/nvme_qpair.c

607

Looks like I'm so busted for copying and pasting, eh? Fixed.

[jrtc27]

I nearly wrote this exact diff rather than the one I ended up with; I didn't, for simplicity, but I do think it's better than my less efficient version.

[jrtc27]

Subject and description will need updating to reflect the final patch before committing though

[imp]

In D31002#697580, @jrtc27 wrote:

Subject and description will need updating to reflect the final patch before committing though

Yes. I'll update, but changing that in mid review messes up git-arc unless done very carefully :(

[jrtc27]

In D31002#697581, @imp wrote:

In D31002#697580, @jrtc27 wrote:

Subject and description will need updating to reflect the final patch before committing though

Yes. I'll update, but changing that in mid review messes up git-arc unless done very carefully :(

Ah, right...