Re: [PATCH v8 13/13] nvmet: Optionally use PCI P2P memory

From: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
To: Logan Gunthorpe <logang-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org,
	linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: "Jens Axboe" <axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>,
	"Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>,
	"Benjamin Herrenschmidt"
	<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>,
	"Steve Wise"
	<swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>,
	"Alex Williamson"
	<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	"Jérôme Glisse" <jglisse-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
	"Jason Gunthorpe" <jgg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>,
	"Bjorn Helgaas"
	<bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
	"Max Gurtovoy" <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>,
	"Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>
Subject: Re: [PATCH v8 13/13] nvmet: Optionally use PCI P2P memory
Date: Mon, 1 Oct 2018 15:23:25 -0700	[thread overview]
Message-ID: <b790b11f-41df-f495-fdd1-d8bd6d6b7dd6@grimberg.me> (raw)
In-Reply-To: <69cd8aab-b94e-98f5-5397-48bb875e8280-OTvnGxWRz7hWk0Htik3J/w@public.gmane.org>

>>> +/*
>>> + * If allow_p2pmem is set, we will try to use P2P memory for the SGL lists for
>>> + * Ι/O commands. This requires the PCI p2p device to be compatible with the
>>> + * backing device for every namespace on this controller.
>>> + */
>>> +static void nvmet_setup_p2pmem(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
>>> +{
>>> +	struct nvmet_ns *ns;
>>> +	int ret;
>>> +
>>> +	if (!req->port->use_p2pmem || !req->p2p_client)
>>> +		return;
>>
>> Nit, IMO would be better to check at the call-site, but not a hard
>> must...
> 
> I'd rather keep the logic for whether to enable p2pmem in it's own
> function. nvme_alloc_ctrl() is already very long and complicated.

Fair enough..

>> I still do not fully understand why p2p_dev has to be ctrl-wide and not
>> per namespace. Sorry to keep bringing this up (again). But if people are
>> OK with it then I guess I can stop asking about this...
> 
> Because you never answered my question back in March[1] (which I think
> you've answered below)....

I'm sorry... I lost tracking on this...

>> I think that at some point we said that this looks like it should fall
>> back to host memory for those namespaces.. when we allocate the sgl we
>> already assigned a namespace to the request (nvmet_req_init).
> 
> I did not realize the namespace would be available at this time. I guess
> I can give this a try, but it's going to be a fairly big change from
> what's presented here... Though, I agree it'll probably be an improvement.

Thanks, if it turns out to create to much of a churn, we could defer
that to a later stage, but we can at least document it.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm