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

Groups > linux.kernel > #1267581

RE: [PATCH 4/5] NVMe: add blk polling support

Show all headers | View raw

> -----Original Message-----
> From: Keith Busch [mailto:keith.busch@intel.com]
> Sent: Friday, November 6, 2015 6:58 PM
> Subject: Re: [PATCH 4/5] NVMe: add blk polling support
> 
> On Fri, Nov 06, 2015 at 03:46:07PM -0800, Elliott, Robert wrote:
> > > -----Original Message-----
> > > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> > > owner@vger.kernel.org] On Behalf Of Jens Axboe
> > > Sent: Friday, November 6, 2015 11:20 AM
> > ...
> > > Subject: [PATCH 4/5] NVMe: add blk polling support
> > >
> > > @@ -953,6 +953,8 @@ static int nvme_process_cq(struct nvme_queue
> *nvmeq)
> > >                       head = 0;
> > >                       phase = !phase;
> > >               }
> > > +             if (tag && *tag == cqe.command_id)
> > > +                     *tag = -1;
> > >               ctx = nvme_finish_cmd(nvmeq, cqe.command_id, &fn);
> > >               fn(nvmeq, ctx, &cqe);
> > >       }
> >
> > The NVMe completion queue entries are 16 bytes long.  Although it's
> > most likely to write them from 0..15 in one PCIe Memory Write
> > transaction, the NVMe device could write those bytes in any order.
> > It could thus update the command identifier before the other bytes,
> > causing this code to process invalid stale values in the other
> > fields.
> 
> That's a very interesting point. We are okay if we can rely on the phase
> bit, which we can by my reading of the spec. Coalescing would not work
> if the driver can observe a new phase in a partially written CQE.

The Phase bit is in the same Dword as the Command Identifier, so
it doesn't help.  If that Dword were written first, then the
preceding three Dwords might not be valid yet.

I'll ask our NVMe representative to propose wording like this:

  4.6 Completion Queue Entry
  ...
  The controller shall write the Dwords in a CQE from lowest to highest 
  (e.g., if the CQE is 16 bytes, write Dword 0, then Dword 1, then
  Dword 2, then Dword 3).  This ensures that the first three Dwords are
  valid when the Phase Tag and Command Identifier are valid. Additional
  Dwords, if any, are not valid at that time.

and refer to that rule in 7.1 where host processing of completions
is discussed.

---
Robert Elliott, HPE Persistent Memory


--
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/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/5] Initial support for polled IO Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
  [PATCH 5/5] directio: add block polling support Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
  [PATCH 1/5] block: change ->make_request_fn() and users to return a queue cookie Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
  [PATCH 3/5] block: add block polling support Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
  [PATCH 2/5] blk-mq: return tag/queue combo in the make_request_fn handlers Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
  [PATCH 4/5] NVMe: add blk polling support Jens Axboe <axboe@fb.com> - 2015-11-06 18:30 +0100
    RE: [PATCH 4/5] NVMe: add blk polling support "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2015-11-07 00:50 +0100
      Re: [PATCH 4/5] NVMe: add blk polling support Keith Busch <keith.busch@intel.com> - 2015-11-07 02:00 +0100
        RE: [PATCH 4/5] NVMe: add blk polling support "Elliott, Robert (Persistent Memory)" <elliott@hpe.com> - 2015-11-12 02:50 +0100
  Re: [PATCH 0/5] Initial support for polled IO Keith Busch <keith.busch@intel.com> - 2015-11-06 23:50 +0100
  Re: [PATCH 0/5] Initial support for polled IO Christoph Hellwig <hch@infradead.org> - 2015-11-07 09:50 +0100

csiph-web