Re: [PATCH v5 07/13] block: Add PCI P2P flag for request queue and check support for requests

From: Jens Axboe <axboe@kernel.dk>
To: Logan Gunthorpe <logang@deltatee.com>, Christoph Hellwig <hch@lst.de>
Cc: "Alex Williamson" <alex.williamson@redhat.com>,
	linux-nvdimm@lists.01.org, linux-rdma@vger.kernel.org,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nvme@lists.infradead.org, linux-block@vger.kernel.org,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Jason Gunthorpe" <jgg@mellanox.com>,
	"Benjamin Herrenschmidt" <benh@kernel.crashing.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Max Gurtovoy" <maxg@mellanox.com>,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [PATCH v5 07/13] block: Add PCI P2P flag for request queue and check support for requests
Date: Wed, 5 Sep 2018 14:36:56 -0600	[thread overview]
Message-ID: <34d9b2f7-9e6d-4a0a-77e5-ec0e99610f5c@kernel.dk> (raw)
In-Reply-To: <f9e660cd-b597-bb6e-56a2-82fe71cb3649@deltatee.com>

On 9/5/18 2:32 PM, Logan Gunthorpe wrote:
> 
> 
> On 05/09/18 02:19 PM, Jens Axboe wrote:
>> On 9/5/18 2:18 PM, Logan Gunthorpe wrote:
>>>
>>>
>>> On 05/09/18 02:14 PM, Jens Axboe wrote:
>>>> But if the caller must absolutely know where the bio will end up, then
>>>> it seems super redundant. So I'd vote for killing this check, it buys
>>>> us absolutely nothing and isn't even exhaustive in its current form.
>>>
>>>
>>> Ok, I'll remove it for v6.
>>
>> Since the drivers needs to know it's doing it right, it might not
>> hurt to add a sanity check helper for that. Just have the driver
>> call it, and don't add it in the normal IO submission path.
> 
> I'm not sure I really see the value in that. It's the same principle in
> asking the driver to do the WARN: if the developer knew enough to use
> the special helper, they probably knew well enough to do the rest correctly.

I don't agree with that at all. It's a "is my request valid" helper,
it's not some obscure and rarely used functionality. You're making up
this API right now, if you really want it done for every IO, make it
part of the p2p submission process. You could even hide it behind a
debug thing, if you like.

> I guess one other thing to point out is that, on x86, if a driver
> submits P2P pages to a PCI device that doesn't have kernel support,
> everything will likely just work. Even though the driver isn't doing any
> of the work correctly and the requests are not being mapped with
> pci_p2pdma_map() functions. Such code on other arches would likely
> break. So developers may be lulled into thinking they're doing the
> correct thing when in fact they are not and the WARN in the common code
> would prevent that.

If you're adamant about having it in common code, put it in your
common submission code. Most folks aren't going to care about P2P, let the
ones that do have the checks.

-- 
Jens Axboe

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm