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

[Sagi Grimberg]

> > > +/*
> > > + * 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.