Re: [PATCH V2 6/6] nvme-pci: remove .init_request callback
Please prepare a formal one(at least tested in normal case), either I
or Zhang Yi may test/verify it.
OK.
@@ -1387,10 +1385,7 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev,
struct nvme_queue *nvmeq,
static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
int depth, int node)
{
- struct nvme_queue *nvmeq = kzalloc_node(sizeof(*nvmeq), GFP_KERNEL,
- node);
- if (!nvmeq)
- return NULL;
+ struct nvme_queue *nvmeq = >queues[qid];
Maybe you need to zero *nvmeq again since it is done in current code.
Relying on this is not a good idea, so I think we better off without
it because I want to know about it and fix it.
@@ -2470,8 +2465,9 @@ static int nvme_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
if (!dev)
return -ENOMEM;
- dev->queues = kzalloc_node((num_possible_cpus() + 1) * sizeof(void
*),
- GFP_KERNEL, node);
+
+ alloc_size = (num_possible_cpus() + 1) * sizeof(struct nvme_queue
*);
The element size should be 'sizeof(struct nvme_queue)'.
Right.
Re: [PATCH V2 6/6] nvme-pci: remove .init_request callback
Hi Sagi,
On Thu, Dec 21, 2017 at 10:20:45AM +0200, Sagi Grimberg wrote:
> Ming,
>
> I'd prefer that we make the pci driver match
> the rest of the drivers in nvme.
OK, this way looks better.
>
> IMO it would be better to allocate a queues array at probe time
> and simply reuse it at reset sequence.
>
> Can this (untested) patch also fix the issue your seeing:
Please prepare a formal one(at least tested in normal case), either I
or Zhang Yi may test/verify it.
> --
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index f26aaa393016..a8edb29dac16 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -75,7 +75,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool
> shutdown);
> * Represents an NVM Express device. Each nvme_dev is a PCI function.
> */
> struct nvme_dev {
> - struct nvme_queue **queues;
> + struct nvme_queue *queues;
> struct blk_mq_tag_set tagset;
> struct blk_mq_tag_set admin_tagset;
> u32 __iomem *dbs;
> @@ -365,7 +365,7 @@ static int nvme_admin_init_hctx(struct blk_mq_hw_ctx
> *hctx, void *data,
> unsigned int hctx_idx)
> {
> struct nvme_dev *dev = data;
> - struct nvme_queue *nvmeq = dev->queues[0];
> + struct nvme_queue *nvmeq = >queues[0];
>
> WARN_ON(hctx_idx != 0);
> WARN_ON(dev->admin_tagset.tags[0] != hctx->tags);
> @@ -387,7 +387,7 @@ static int nvme_init_hctx(struct blk_mq_hw_ctx *hctx,
> void *data,
> unsigned int hctx_idx)
> {
> struct nvme_dev *dev = data;
> - struct nvme_queue *nvmeq = dev->queues[hctx_idx + 1];
> + struct nvme_queue *nvmeq = >queues[hctx_idx + 1];
>
> if (!nvmeq->tags)
> nvmeq->tags = >tagset.tags[hctx_idx];
> @@ -403,7 +403,7 @@ static int nvme_init_request(struct blk_mq_tag_set *set,
> struct request *req,
> struct nvme_dev *dev = set->driver_data;
> struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> int queue_idx = (set == >tagset) ? hctx_idx + 1 : 0;
> - struct nvme_queue *nvmeq = dev->queues[queue_idx];
> + struct nvme_queue *nvmeq = >queues[queue_idx];
>
> BUG_ON(!nvmeq);
> iod->nvmeq = nvmeq;
> @@ -1046,7 +1046,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx,
> unsigned int tag)
> static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
> {
> struct nvme_dev *dev = to_nvme_dev(ctrl);
> - struct nvme_queue *nvmeq = dev->queues[0];
> + struct nvme_queue *nvmeq = >queues[0];
> struct nvme_command c;
>
> memset(, 0, sizeof(c));
> @@ -1290,10 +1290,8 @@ static void nvme_free_queues(struct nvme_dev *dev,
> int lowest)
> int i;
>
> for (i = dev->ctrl.queue_count - 1; i >= lowest; i--) {
> - struct nvme_queue *nvmeq = dev->queues[i];
> dev->ctrl.queue_count--;
> - dev->queues[i] = NULL;
> - nvme_free_queue(nvmeq);
> + nvme_free_queue(>queues[i]);
> }
> }
>
> @@ -1325,7 +1323,7 @@ static int nvme_suspend_queue(struct nvme_queue
> *nvmeq)
>
> static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
> {
> - struct nvme_queue *nvmeq = dev->queues[0];
> + struct nvme_queue *nvmeq = >queues[0];
>
> if (!nvmeq)
> return;
> @@ -1387,10 +1385,7 @@ static int nvme_alloc_sq_cmds(struct nvme_dev *dev,
> struct nvme_queue *nvmeq,
> static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
> int depth, int node)
> {
> - struct nvme_queue *nvmeq = kzalloc_node(sizeof(*nvmeq), GFP_KERNEL,
> - node);
> - if (!nvmeq)
> - return NULL;
> + struct nvme_queue *nvmeq = >queues[qid];
Maybe you need to zero *nvmeq again since it is done in current code.
>
> nvmeq->cqes = dma_zalloc_coherent(dev->dev, CQ_SIZE(depth),
> >cq_dma_addr, GFP_KERNEL);
> @@ -1409,7 +1404,6 @@ static struct nvme_queue *nvme_alloc_queue(struct
> nvme_dev *dev, int qid,
> nvmeq->q_depth = depth;
> nvmeq->qid = qid;
> nvmeq->cq_vector = -1;
> - dev->queues[qid] = nvmeq;
> dev->ctrl.queue_count++;
>
> return nvmeq;
> @@ -1592,7 +1586,7 @@ static int nvme_pci_configure_admin_queue(struct
> nvme_dev *dev)
> if (result < 0)
> return result;
>
> - nvmeq = dev->queues[0];
> + nvmeq = >queues[0];
> if (!nvmeq) {
> nvmeq = nvme_alloc_queue(dev, 0, NVME_AQ_DEPTH,
> dev_to_node(dev->dev));
> @@ -1638,7 +1632,7 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
>
> max = min(dev->max_qid, dev->ctrl.queue_count - 1);
> for (i = dev->online_queues; i <= max;
Re: [PATCH V2 6/6] nvme-pci: remove .init_request callback
On Thu, Dec 21, 2017 at 10:20:45AM +0200, Sagi Grimberg wrote: > @@ -2470,8 +2465,9 @@ static int nvme_probe(struct pci_dev *pdev, const > struct pci_device_id *id) > dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node); > if (!dev) > return -ENOMEM; > - dev->queues = kzalloc_node((num_possible_cpus() + 1) * sizeof(void > *), > - GFP_KERNEL, node); > + > + alloc_size = (num_possible_cpus() + 1) * sizeof(struct nvme_queue > *); > + dev->queues = kzalloc_node(alloc_size, GFP_KERNEL, node); Nit: dev->queues = kcalloc_node(num_possible_cpus() + 1, sizeof(struct nvme_queue *), node); -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH V2 6/6] nvme-pci: remove .init_request callback
Ming,
I'd prefer that we make the pci driver match
the rest of the drivers in nvme.
IMO it would be better to allocate a queues array at probe time
and simply reuse it at reset sequence.
Can this (untested) patch also fix the issue your seeing:
--
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f26aaa393016..a8edb29dac16 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -75,7 +75,7 @@ static void nvme_dev_disable(struct nvme_dev *dev,
bool shutdown);
* Represents an NVM Express device. Each nvme_dev is a PCI function.
*/
struct nvme_dev {
- struct nvme_queue **queues;
+ struct nvme_queue *queues;
struct blk_mq_tag_set tagset;
struct blk_mq_tag_set admin_tagset;
u32 __iomem *dbs;
@@ -365,7 +365,7 @@ static int nvme_admin_init_hctx(struct blk_mq_hw_ctx
*hctx, void *data,
unsigned int hctx_idx)
{
struct nvme_dev *dev = data;
- struct nvme_queue *nvmeq = dev->queues[0];
+ struct nvme_queue *nvmeq = >queues[0];
WARN_ON(hctx_idx != 0);
WARN_ON(dev->admin_tagset.tags[0] != hctx->tags);
@@ -387,7 +387,7 @@ static int nvme_init_hctx(struct blk_mq_hw_ctx
*hctx, void *data,
unsigned int hctx_idx)
{
struct nvme_dev *dev = data;
- struct nvme_queue *nvmeq = dev->queues[hctx_idx + 1];
+ struct nvme_queue *nvmeq = >queues[hctx_idx + 1];
if (!nvmeq->tags)
nvmeq->tags = >tagset.tags[hctx_idx];
@@ -403,7 +403,7 @@ static int nvme_init_request(struct blk_mq_tag_set
*set, struct request *req,
struct nvme_dev *dev = set->driver_data;
struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
int queue_idx = (set == >tagset) ? hctx_idx + 1 : 0;
- struct nvme_queue *nvmeq = dev->queues[queue_idx];
+ struct nvme_queue *nvmeq = >queues[queue_idx];
BUG_ON(!nvmeq);
iod->nvmeq = nvmeq;
@@ -1046,7 +1046,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx,
unsigned int tag)
static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
{
struct nvme_dev *dev = to_nvme_dev(ctrl);
- struct nvme_queue *nvmeq = dev->queues[0];
+ struct nvme_queue *nvmeq = >queues[0];
struct nvme_command c;
memset(, 0, sizeof(c));
@@ -1290,10 +1290,8 @@ static void nvme_free_queues(struct nvme_dev
*dev, int lowest)
int i;
for (i = dev->ctrl.queue_count - 1; i >= lowest; i--) {
- struct nvme_queue *nvmeq = dev->queues[i];
dev->ctrl.queue_count--;
- dev->queues[i] = NULL;
- nvme_free_queue(nvmeq);
+ nvme_free_queue(>queues[i]);
}
}
@@ -1325,7 +1323,7 @@ static int nvme_suspend_queue(struct nvme_queue
*nvmeq)
static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
{
- struct nvme_queue *nvmeq = dev->queues[0];
+ struct nvme_queue *nvmeq = >queues[0];
if (!nvmeq)
return;
@@ -1387,10 +1385,7 @@ static int nvme_alloc_sq_cmds(struct nvme_dev
*dev, struct nvme_queue *nvmeq,
static struct nvme_queue *nvme_alloc_queue(struct nvme_dev *dev, int qid,
int depth, int
node)
{
- struct nvme_queue *nvmeq = kzalloc_node(sizeof(*nvmeq), GFP_KERNEL,
- node);
- if (!nvmeq)
- return NULL;
+ struct nvme_queue *nvmeq = >queues[qid];
nvmeq->cqes = dma_zalloc_coherent(dev->dev, CQ_SIZE(depth),
>cq_dma_addr, GFP_KERNEL);
@@ -1409,7 +1404,6 @@ static struct nvme_queue *nvme_alloc_queue(struct
nvme_dev *dev, int qid,
nvmeq->q_depth = depth;
nvmeq->qid = qid;
nvmeq->cq_vector = -1;
- dev->queues[qid] = nvmeq;
dev->ctrl.queue_count++;
return nvmeq;
@@ -1592,7 +1586,7 @@ static int nvme_pci_configure_admin_queue(struct
nvme_dev *dev)
if (result < 0)
return result;
- nvmeq = dev->queues[0];
+ nvmeq = >queues[0];
if (!nvmeq) {
nvmeq = nvme_alloc_queue(dev, 0, NVME_AQ_DEPTH,
dev_to_node(dev->dev));
@@ -1638,7 +1632,7 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
max = min(dev->max_qid, dev->ctrl.queue_count - 1);
for (i = dev->online_queues; i <= max; i++) {
- ret = nvme_create_queue(dev->queues[i], i);
+ ret = nvme_create_queue(>queues[i], i);
if (ret)
break;
}
@@ -1894,7 +1888,7 @@ static int nvme_setup_host_mem(struct nvme_dev *dev)
static int nvme_setup_io_queues(struct nvme_dev *dev)
{
- struct nvme_queue *adminq = dev->queues[0];
+ struct nvme_queue *adminq = >queues[0];
struct pci_dev *pdev = to_pci_dev(dev->dev);
int result,
