linux-kernel - Re: [PATCH v3 11/11] nvmet: Optionally use PCI P2P memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Mar 2018 10:52:25 -0600
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Christoph Hellwig <hch@....de>
Cc:     linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-rdma@...r.kernel.org,
        linux-nvdimm@...ts.01.org, linux-block@...r.kernel.org,
        Stephen Bates <sbates@...thlin.com>,
        Jens Axboe <axboe@...nel.dk>,
        Keith Busch <keith.busch@...el.com>,
        Sagi Grimberg <sagi@...mberg.me>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Jason Gunthorpe <jgg@...lanox.com>,
        Max Gurtovoy <maxg@...lanox.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Jérôme Glisse <jglisse@...hat.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Steve Wise <swise@...ngridcomputing.com>
Subject: Re: [PATCH v3 11/11] nvmet: Optionally use PCI P2P memory



On 21/03/18 03:27 AM, Christoph Hellwig wrote:
>> +				  const char *page, size_t count)
>> +{
>> +	struct nvmet_port *port = to_nvmet_port(item);
>> +	struct device *dev;
>> +	struct pci_dev *p2p_dev = NULL;
>> +	bool use_p2pmem;
>> +
>> +	switch (page[0]) {
>> +	case 'y':
>> +	case 'Y':
>> +	case 'a':
>> +	case 'A':
>> +		use_p2pmem = true;
>> +		break;
>> +	case 'n':
>> +	case 'N':
>> +		use_p2pmem = false;
>> +		break;
>> +	default:
>> +		dev = bus_find_device_by_name(&pci_bus_type, NULL, page);
>> +		if (!dev) {
>> +			pr_err("No such PCI device: %s\n", page);
>> +			return -ENODEV;
>> +		}
>> +
>> +		use_p2pmem = true;
>> +		p2p_dev = to_pci_dev(dev);
>> +
>> +		if (!pci_has_p2pmem(p2p_dev)) {
>> +			pr_err("PCI device has no peer-to-peer memory: %s\n",
>> +			       page);
>> +			pci_dev_put(p2p_dev);
>> +			return -ENODEV;
>> +		}
>> +	}
> 
> Yikes.  Shouldn't auto just be the normal yes case instead of this
> string parsing mess?

Sorry, I don't follow. The code, as is, should automatically select the
device if the user  sets it to "yes" or "auto" or "y" or similar.
(Roughly similar to how kstrtobool() works, except '0' or '1' are not
accepted seeing they could overlap with PCI device names). In other
cases, it looks for the specific PCI device name to use exactly.

Are you saying it shouldn't work this way or are you saying the code to
implement it is too messy?

>> +	if (rsp->req.sg != &rsp->cmd->inline_sg) {
>> +		if (rsp->req.p2p_dev)
>> +			pci_p2pmem_free_sgl(rsp->req.p2p_dev, rsp->req.sg,
>> +					    rsp->req.sg_cnt);
>> +		else
>> +			sgl_free(rsp->req.sg);
>> +	}
> 
> Can we factor this into a helper, as the other target drivers (fc for now,
> tcp soon) using sgl allocatins should share the code?
> 
> (same for the alloc side)

Sure. Would the helpers belong in core.c?

Thanks,

Logan