Re: [PATCH] nvme-pci: slimmer CQ head update

From: Robin Murphy <robin.murphy@arm.com>
To: Keith Busch <kbusch@kernel.org>, John Garry <john.garry@huawei.com>
Cc: sagi@grimberg.me, linux-nvme@lists.infradead.org,
	Alexey Dobriyan <adobriyan@gmail.com>,
	axboe@fb.com, Will Deacon <will@kernel.org>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH] nvme-pci: slimmer CQ head update
Date: Thu, 7 May 2020 17:26:58 +0100	[thread overview]
Message-ID: <9e04eebc-a55b-ac7e-3cb3-9c65f084ee6c@arm.com> (raw)
In-Reply-To: <20200507153506.GA2621480@dhcp-10-100-145-180.wdl.wdc.com>

On 2020-05-07 4:35 pm, Keith Busch wrote:
> On Thu, May 07, 2020 at 04:11:23PM +0100, John Garry wrote:
>> On 07/05/2020 15:23, Keith Busch wrote:
>>> On Thu, May 07, 2020 at 02:55:37PM +0100, John Garry wrote:
>>>> On 07/05/2020 12:04, Robin Murphy wrote:
>>>>>> [  177.132810] DMA-API: nvme 0000:85:00.0: device driver tries to
>>>>>> free DMA memor
>>>>>> y it has not allocated [device address=0x00000000ef371000]
>>>>>> [size=4096 bytes]
>>>>> [...]
>>>>>> [  177.276322]  debug_dma_unmap_page+0x6c/0x78
>>>>>> [  177.280487]  nvme_unmap_data+0x7c/0x23c
>>>>>> [  177.284305]  nvme_pci_complete_rq+0x28/0x58
>>>>>
>>>>> OK, so there's clearly something amiss there. I would have suggested
>>>>> next sticking the SMMU in passthrough to help focus on the DMA API
>>>>> debugging, but since that "DMA address" looks suspiciously like a
>>>>> physical address rather than an IOVA, I suspect that things might
>>>>> suddenly appear to be working fine if you do...
>>>>
>>>> OK, seems sensible. However it looks like this guy triggers the issue:
>>>>
>>>> 324b494c2862 nvme-pci: Remove two-pass completions
>>>>
>>>> With carrying the revert of $subject, it's a quick bisect to that patch.
>>>
>>> That's weird.
>>
>> Or maybe exacerbating some other fault?
>>
>>   Do you see this with different nvme controllers?
>>
>> I only have 3x, and they are all ES3000 V3 NVMe PCIe SSD
>>
>>> Does your
>>> controller write the phase bit before writing the command id in the cqe?
>>
>> I don't know. Is that sort of info available from nvme-cli?
> 
> No, the only way to 100% confirm is with bus protocol analyzers. It's
> a protocol violation if a controller was behaving that way. We've seen
> devices broken like that before, though it's been a while since I've
> seen such behvaior.
> 
>> [  885.344575] WARNING: CPU: 41 PID: 4565 at block/blk-mq.c:665  blk_mq_start_request+0xc4/0xcc
> 
> This warning appears to support my suspicion: the completion side is
> observing a new phase with a stale command id, and that command id was
> reallocated as a new request that we're still constructing at the time
> the double-completion occured.
> 
> Host software is supposed to be guaranteed the entire CQE is written
> once we see an updated phase, per spec: "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."

Hmm, that makes me wonder if there might be some interaction with the 
Arm memory model here - if there are strict ordering requirements for 
things in memory being observed, could we be missing appropriate 
barriers between reads/writes of the various fields?

Robin.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme