NVMe Async Event Notification updates

PATCH New Keith Busch 7 participants 37 messages Jul 7, 2017 – Aug 1, 2017

Subsystems (3)

NVMe NVMe target NVMe FC transport

lore

Share & tools

lore

Keith Busch

author

2017-07-07 16:22 UTC #0 lore raw

cover NVMe Async Event Notification updates

This culminates with udev handling for AEN, but implementing this was much easier without all the unreachable handling for multiple AEN requests.

Some background on why single AEN's are used today: we can't tell blk-mq about those reserved tags lest we deadlock a freeze. These requests are not worth the trouble to implement tag accounting required for multiple requests outstanding, and one AEN request is sufficient for all uses anyway.

I don't have a setup to try the nvme/fc parts, so while I think this looks correct, it's only compile tested.

Keith Busch (7):
nvme/fc: There is only one AEN request
nvme/fc: Fix admin queue depth setup
nvme/fc: remove unused "queue_size" field
nvme: Centralize blk-mq tag and AEN counts
nvme: Only one AEN request
nvme: Unexport starting async event work
nvme: Send change uevent when AEN completes

 drivers/nvme/host/core.c   |  46 ++++++------
 drivers/nvme/host/fc.c     | 175 ++++++++++++++++++---------------------------
 drivers/nvme/host/nvme.h   |   6 +-
 drivers/nvme/host/pci.c    |  10 +--
 drivers/nvme/host/rdma.c   |  19 ++---
 drivers/nvme/target/loop.c |  16 ++---
 include/linux/nvme.h       |   7 ++
 7 files changed, 107 insertions(+), 172 deletions(-)

Signature · 1 line

2.5.5

Keith Busch

author

2017-07-07 16:22 UTC #1 lore raw

PATCH 1/7 nvme/fc: There is only one AEN request

This patch removes the multiple AEN request handling. There can be only one.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/fc.c | 145 +++++++++++++++++++++----------------------------
 1 file changed, 61 insertions(+), 84 deletions(-)

drivers/nvme/host/fc.c +61 ~~-84~~

index d666ada..8355937 100644
@@ -165,7 +165,7 @@ struct nvme_fc_ctrl {
 	u32			iocnt;
 	wait_queue_head_t	ioabort_wait;
 
-	struct nvme_fc_fcp_op	aen_ops[NVME_FC_NR_AEN_COMMANDS];
+	struct nvme_fc_fcp_op	aen_op;
 
 	struct nvme_ctrl	ctrl;
 };

@@ -1192,39 +1192,33 @@ __nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
 }
 
 static void
-nvme_fc_abort_aen_ops(struct nvme_fc_ctrl *ctrl)
+nvme_fc_abort_aen_op(struct nvme_fc_ctrl *ctrl)
 {
-	struct nvme_fc_fcp_op *aen_op = ctrl->aen_ops;
 	unsigned long flags;
-	int i, ret;
 
-	for (i = 0; i < NVME_FC_NR_AEN_COMMANDS; i++, aen_op++) {
-		if (atomic_read(&aen_op->state) != FCPOP_STATE_ACTIVE)
-			continue;
+	if (atomic_read(&ctrl->aen_op.state) != FCPOP_STATE_ACTIVE)
+		return;
 
+	spin_lock_irqsave(&ctrl->lock, flags);
+	if (ctrl->flags & FCCTRL_TERMIO) {
+		ctrl->iocnt++;
+		ctrl->aen_op.flags |= FCOP_FLAGS_TERMIO;
+	}
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	if (__nvme_fc_abort_op(ctrl, &ctrl->aen_op)) {
+		/*
+		 * if __nvme_fc_abort_op failed the io wasn't
+		 * active. Thus this call path is running in
+		 * parallel to the io complete. Treat as non-error.
+		 */
+
+		/* back out the flags/counters */
 		spin_lock_irqsave(&ctrl->lock, flags);
-		if (ctrl->flags & FCCTRL_TERMIO) {
-			ctrl->iocnt++;
-			aen_op->flags |= FCOP_FLAGS_TERMIO;
-		}
+		if (ctrl->flags & FCCTRL_TERMIO)
+			ctrl->iocnt--;
+		ctrl->aen_op.flags &= ~FCOP_FLAGS_TERMIO;
 		spin_unlock_irqrestore(&ctrl->lock, flags);
-
-		ret = __nvme_fc_abort_op(ctrl, aen_op);
-		if (ret) {
-			/*
-			 * if __nvme_fc_abort_op failed the io wasn't
-			 * active. Thus this call path is running in
-			 * parallel to the io complete. Treat as non-error.
-			 */
-
-			/* back out the flags/counters */
-			spin_lock_irqsave(&ctrl->lock, flags);
-			if (ctrl->flags & FCCTRL_TERMIO)
-				ctrl->iocnt--;
-			aen_op->flags &= ~FCOP_FLAGS_TERMIO;
-			spin_unlock_irqrestore(&ctrl->lock, flags);
-			return;
-		}
 	}
 }

@@ -1454,59 +1448,49 @@ nvme_fc_init_request(struct blk_mq_tag_set *set, struct request *rq,
 }
 
 static int
-nvme_fc_init_aen_ops(struct nvme_fc_ctrl *ctrl)
+nvme_fc_init_aen_op(struct nvme_fc_ctrl *ctrl)
 {
-	struct nvme_fc_fcp_op *aen_op;
 	struct nvme_fc_cmd_iu *cmdiu;
 	struct nvme_command *sqe;
 	void *private;
-	int i, ret;
-
-	aen_op = ctrl->aen_ops;
-	for (i = 0; i < NVME_FC_NR_AEN_COMMANDS; i++, aen_op++) {
-		private = kzalloc(ctrl->lport->ops->fcprqst_priv_sz,
-						GFP_KERNEL);
-		if (!private)
-			return -ENOMEM;
-
-		cmdiu = &aen_op->cmd_iu;
-		sqe = &cmdiu->sqe;
-		ret = __nvme_fc_init_request(ctrl, &ctrl->queues[0],
-				aen_op, (struct request *)NULL,
-				(AEN_CMDID_BASE + i));
-		if (ret) {
-			kfree(private);
-			return ret;
-		}
+	int ret;
 
-		aen_op->flags = FCOP_FLAGS_AEN;
-		aen_op->fcp_req.first_sgl = NULL; /* no sg list */
-		aen_op->fcp_req.private = private;
+	private = kzalloc(ctrl->lport->ops->fcprqst_priv_sz,
+					GFP_KERNEL);
+	if (!private)
+		return -ENOMEM;
 
-		memset(sqe, 0, sizeof(*sqe));
-		sqe->common.opcode = nvme_admin_async_event;
-		/* Note: core layer may overwrite the sqe.command_id value */
-		sqe->common.command_id = AEN_CMDID_BASE + i;
+	cmdiu = &ctrl->aen_op.cmd_iu;
+	sqe = &cmdiu->sqe;
+	ret = __nvme_fc_init_request(ctrl, &ctrl->queues[0],
+			&ctrl->aen_op, (struct request *)NULL,
+			AEN_CMDID_BASE);
+	if (ret) {
+		kfree(private);
+		return ret;
 	}
+
+	ctrl->aen_op.flags = FCOP_FLAGS_AEN;
+	ctrl->aen_op.fcp_req.first_sgl = NULL; /* no sg list */
+	ctrl->aen_op.fcp_req.private = private;
+
+	memset(sqe, 0, sizeof(*sqe));
+	sqe->common.opcode = nvme_admin_async_event;
+	sqe->common.command_id = AEN_CMDID_BASE;
+
 	return 0;
 }
 
 static void
-nvme_fc_term_aen_ops(struct nvme_fc_ctrl *ctrl)
+nvme_fc_term_aen_op(struct nvme_fc_ctrl *ctrl)
 {
-	struct nvme_fc_fcp_op *aen_op;
-	int i;
-
-	aen_op = ctrl->aen_ops;
-	for (i = 0; i < NVME_FC_NR_AEN_COMMANDS; i++, aen_op++) {
-		if (!aen_op->fcp_req.private)
-			continue;
+	if (!ctrl->aen_op.fcp_req.private)
+		return;
 
-		__nvme_fc_exit_request(ctrl, aen_op);
+	__nvme_fc_exit_request(ctrl, &ctrl->aen_op);
 
-		kfree(aen_op->fcp_req.private);
-		aen_op->fcp_req.private = NULL;
-	}
+	kfree(ctrl->aen_op.fcp_req.private);
+	ctrl->aen_op.fcp_req.private = NULL;
 }
 
 static inline void

@@ -2041,10 +2025,8 @@ static void
 nvme_fc_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 {
 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(arg);
-	struct nvme_fc_fcp_op *aen_op;
 	unsigned long flags;
 	bool terminating = false;
-	blk_status_t ret;
 
 	if (aer_idx > NVME_FC_NR_AEN_COMMANDS)
 		return;

@@ -2056,14 +2038,9 @@ nvme_fc_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 
 	if (terminating)
 		return;
-
-	aen_op = &ctrl->aen_ops[aer_idx];
-
-	ret = nvme_fc_start_fcp_op(ctrl, aen_op->queue, aen_op, 0,
-					NVMEFC_FCP_NODATA);
-	if (ret)
-		dev_err(ctrl->ctrl.device,
-			"failed async event work [%d]\n", aer_idx);
+	if (nvme_fc_start_fcp_op(ctrl, &ctrl->queues[0], &ctrl->aen_op, 0,
+						NVMEFC_FCP_NODATA))
+		dev_err(ctrl->ctrl.device, "failed async event work\n");
 }
 
 static void

@@ -2376,9 +2353,9 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 		opts->queue_size = ctrl->ctrl.maxcmd;
 	}
 
-	ret = nvme_fc_init_aen_ops(ctrl);
+	ret = nvme_fc_init_aen_op(ctrl);
 	if (ret)
-		goto out_term_aen_ops;
+		goto out_term_aen_op;
 
 	/*
 	 * Create the io queues

@@ -2390,7 +2367,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 		else
 			ret = nvme_fc_reinit_io_queues(ctrl);
 		if (ret)
-			goto out_term_aen_ops;
+			goto out_term_aen_op;
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);

@@ -2402,8 +2379,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 
 	return 0;	/* Success */
 
-out_term_aen_ops:
-	nvme_fc_term_aen_ops(ctrl);
+out_term_aen_op:
+	nvme_fc_term_aen_op(ctrl);
 out_disconnect_admin_queue:
 	/* send a Disconnect(association) LS to fc-nvme target */
 	nvme_fc_xmt_disconnect_assoc(ctrl);

@@ -2471,7 +2448,7 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 				nvme_fc_terminate_exchange, &ctrl->ctrl);
 
 	/* kill the aens as they are a separate path */
-	nvme_fc_abort_aen_ops(ctrl);
+	nvme_fc_abort_aen_op(ctrl);
 
 	/* wait for all io that had to be aborted */
 	spin_lock_irqsave(&ctrl->lock, flags);

@@ -2479,7 +2456,7 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 	ctrl->flags &= ~FCCTRL_TERMIO;
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
-	nvme_fc_term_aen_ops(ctrl);
+	nvme_fc_term_aen_op(ctrl);
 
 	/*
 	 * send a Disconnect(association) LS to fc-nvme target

Signature · 1 line

2.5.5

James Smart

2017-07-10 23:12 UTC #2 lore raw

PATCH 1/7 nvme/fc: There is only one AEN request

Quoted 3 lines · Keith Busch

This patch removes the multiple AEN request handling. There can be only one.

Looks fine, although my style would have defined a local variable of aen_op rather than continued use of ctrl->aen_op.

Reviewed-by James Smart <james.smart at broadcom.com>

-- james

Keith Busch

author

2017-07-07 16:22 UTC #3 lore raw

PATCH 2/7 nvme/fc: Fix admin queue depth setup

In reply to Keith Busch

The NVME_FC_AQ_BLKMQ_DEPTH is one less than what we actually want the admin queue's depth to be since it subtracts the AEN entry.

We also need to subtract 1 from the blk-mq depth to always leave an empty queue entry, and to not collide command id with the AEN request.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/fc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/nvme/host/fc.c +4 -5

index 8355937..6328158 100644
@@ -2284,16 +2284,15 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	 * Create the admin queue
 	 */
 
-	nvme_fc_init_queue(ctrl, 0, NVME_FC_AQ_BLKMQ_DEPTH);
+	nvme_fc_init_queue(ctrl, 0, NVME_AQ_DEPTH);
 
 	ret = __nvme_fc_create_hw_queue(ctrl, &ctrl->queues[0], 0,
-				NVME_FC_AQ_BLKMQ_DEPTH);
+				NVME_AQ_DEPTH);
 	if (ret)
 		goto out_free_queue;
 
 	ret = nvme_fc_connect_admin_queue(ctrl, &ctrl->queues[0],
-				NVME_FC_AQ_BLKMQ_DEPTH,
-				(NVME_FC_AQ_BLKMQ_DEPTH / 4));
+				NVME_AQ_DEPTH, (NVME_AQ_DEPTH / 4));
 	if (ret)
 		goto out_delete_hw_queue;

@@ -2693,7 +2692,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
 	ctrl->admin_tag_set.ops = &nvme_fc_admin_mq_ops;
-	ctrl->admin_tag_set.queue_depth = NVME_FC_AQ_BLKMQ_DEPTH;
+	ctrl->admin_tag_set.queue_depth = NVME_FC_AQ_BLKMQ_DEPTH - 1;
 	ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect + Keep-Alive */
 	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +

Signature · 1 line

2.5.5

Sagi Grimberg

maintainer

2017-07-10 05:59 UTC #4 lore raw

PATCH 2/7 nvme/fc: Fix admin queue depth setup

Quoted 7 lines

The NVME_FC_AQ_BLKMQ_DEPTH is one less than what we actually want the admin queue's depth to be since it subtracts the AEN entry.
We also need to subtract 1 from the blk-mq depth to always leave an empty queue entry, and to not collide command id with the AEN request.
Signed-off-by Keith Busch <keith.busch at intel.com>

nvme-loop needs the same patch.

Keith Busch

author

2017-07-10 22:57 UTC #5 lore raw

PATCH 2/7 nvme/fc: Fix admin queue depth setup

Quoted 9 lines · Sagi Grimberg

Quoted 7 lines
The NVME_FC_AQ_BLKMQ_DEPTH is one less than what we actually want the admin queue's depth to be since it subtracts the AEN entry.
We also need to subtract 1 from the blk-mq depth to always leave an empty queue entry, and to not collide command id with the AEN request.
Signed-off-by Keith Busch <keith.busch at intel.com>
nvme-loop needs the same patch.

Yeah, it looks like it does, though it's inadvertently fixed in patch
4. In truth, patch 4 makes this one unnecessary for fc too, but I didn't want to change the q-depth and centralize its definition in a single patch. I'll spin a new version with the loop fix in the series, as well as Christoph's suggestions.

James Smart

2017-07-10 23:40 UTC #6 lore raw

PATCH 2/7 nvme/fc: Fix admin queue depth setup

In reply to Keith Busch

Quoted 5 lines · Keith Busch

The NVME_FC_AQ_BLKMQ_DEPTH is one less than what we actually want the admin queue's depth to be since it subtracts the AEN entry.
We also need to subtract 1 from the blk-mq depth to always leave an empty queue entry, and to not collide command id with the AEN request.

I don't have an issue with what changed, but have a few comments.

Why not kill the NVME_FC_NR_AEN_COMMANDS define and correct the NVME_FC_AQ_BLKMQ_DEPTH define with NVME_NR_AERS as well ?

Given AEN_CMDID_BASE is NVME_FC_AQ_BLKMQ_DEPTH + 1 there never should have been a collision regardless.

Agree with the -1 for empty queue levels. Although the reserved tag for Connect assured it was never an issue.

Note: the logic for the above are in the rdma transport as well.

Reviewed-by James Smart <james.smart at broadcom.com>

-- james

James Smart

2017-07-10 23:49 UTC #7 lore raw

PATCH 2/7 nvme/fc: Fix admin queue depth setup

Quoted 15 lines · James Smart

Quoted 5 lines · Keith Busch
The NVME_FC_AQ_BLKMQ_DEPTH is one less than what we actually want the admin queue's depth to be since it subtracts the AEN entry.
We also need to subtract 1 from the blk-mq depth to always leave an empty queue entry, and to not collide command id with the AEN request.
I don't have an issue with what changed, but have a few comments.
Why not kill the NVME_FC_NR_AEN_COMMANDS define and correct the NVME_FC_AQ_BLKMQ_DEPTH define with NVME_NR_AERS as well ?
Given AEN_CMDID_BASE is NVME_FC_AQ_BLKMQ_DEPTH + 1 there never should have been a collision regardless.

Quoted 9 lines

Agree with the -1 for empty queue levels. Although the reserved tag for Connect assured it was never an issue.
Note: the logic for the above are in the rdma transport as well.
Reviewed-by James Smart <james.smart at broadcom.com>
-- james

ah - patch 4 did what I suggested. Looks good.

Keith Busch

author

2017-07-07 16:22 UTC #8 lore raw

PATCH 3/7 nvme/fc: remove unused "queue_size" field

In reply to Keith Busch

This was being saved in a structure, but never used anywhere. The queue size is obtained through other means, so there's no reason to duplicate this without a user for it.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/fc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

drivers/nvme/host/fc.c +3 -6

index 6328158..26a3c5d 100644
@@ -50,7 +50,6 @@ struct nvme_fc_queue {
 	struct device		*dev;
 	struct blk_mq_hw_ctx	*hctx;
 	void			*lldd_handle;
-	int			queue_size;
 	size_t			cmnd_capsule_len;
 	u32			qnum;
 	u32			rqcnt;

@@ -1526,7 +1525,7 @@ nvme_fc_init_admin_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 }
 
 static void
-nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx, size_t queue_size)
+nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx)
 {
 	struct nvme_fc_queue *queue;

@@ -1542,8 +1541,6 @@ nvme_fc_init_queue(struct nvme_fc_ctrl *ctrl, int idx, size_t queue_size)
 	else
 		queue->cmnd_capsule_len = sizeof(struct nvme_command);
 
-	queue->queue_size = queue_size;
-
 	/*
 	 * Considered whether we should allocate buffers for all SQEs
 	 * and CQEs and dma map them - mapping their respective entries

@@ -1667,7 +1664,7 @@ nvme_fc_init_io_queues(struct nvme_fc_ctrl *ctrl)
 	int i;
 
 	for (i = 1; i < ctrl->ctrl.queue_count; i++)
-		nvme_fc_init_queue(ctrl, i, ctrl->ctrl.sqsize);
+		nvme_fc_init_queue(ctrl, i);
 }
 
 static void

@@ -2284,7 +2281,7 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	 * Create the admin queue
 	 */
 
-	nvme_fc_init_queue(ctrl, 0, NVME_AQ_DEPTH);
+	nvme_fc_init_queue(ctrl, 0);
 
 	ret = __nvme_fc_create_hw_queue(ctrl, &ctrl->queues[0], 0,
 				NVME_AQ_DEPTH);

Signature · 1 line

2.5.5

Sagi Grimberg

maintainer

2017-07-10 06:00 UTC #9 lore raw

PATCH 3/7 nvme/fc: remove unused "queue_size" field

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

James Smart

2017-07-10 23:44 UTC #10 lore raw

PATCH 3/7 nvme/fc: remove unused "queue_size" field

In reply to Keith Busch

Quoted 4 lines · Keith Busch

This was being saved in a structure, but never used anywhere. The queue size is obtained through other means, so there's no reason to duplicate this without a user for it.

Looks good

Reviewed-by James Smart <james.smart at broadcom.com>

Keith Busch

author

2017-07-07 16:22 UTC #11 lore raw

PATCH 4/7 nvme: Centralize blk-mq tag and AEN counts

In reply to Keith Busch

All the drivers were duplicating AEN accounting, yet they all depend on the core to use the exact same settings of a single AEN to be deducted from the tagset depth. This patch moves admin queue command accounting to the core.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/fc.c     | 17 ++++-------------
 drivers/nvme/host/nvme.h   |  1 -
 drivers/nvme/host/pci.c    |  6 ------
 drivers/nvme/host/rdma.c   | 14 +++-----------
 drivers/nvme/target/loop.c | 14 +++-----------
 include/linux/nvme.h       |  7 +++++++
 6 files changed, 17 insertions(+), 42 deletions(-)

drivers/nvme/host/fc.c +4 ~~-13~~

index 26a3c5d..753761c 100644
@@ -30,15 +30,6 @@
 /* *************************** Data Structures/Defines ****************** */
 
 
-/*
- * We handle AEN commands ourselves and don't even let the
- * block layer know about them.
- */
-#define NVME_FC_NR_AEN_COMMANDS	1
-#define NVME_FC_AQ_BLKMQ_DEPTH	\
-	(NVME_AQ_DEPTH - NVME_FC_NR_AEN_COMMANDS)
-#define AEN_CMDID_BASE		(NVME_FC_AQ_BLKMQ_DEPTH + 1)
-
 enum nvme_fc_queue_flags {
 	NVME_FC_Q_CONNECTED = (1 << 0),
 };

@@ -1463,7 +1454,7 @@ nvme_fc_init_aen_op(struct nvme_fc_ctrl *ctrl)
 	sqe = &cmdiu->sqe;
 	ret = __nvme_fc_init_request(ctrl, &ctrl->queues[0],
 			&ctrl->aen_op, (struct request *)NULL,
-			AEN_CMDID_BASE);
+			NVME_AQ_BLKMQ_DEPTH);
 	if (ret) {
 		kfree(private);
 		return ret;

@@ -1475,7 +1466,7 @@ nvme_fc_init_aen_op(struct nvme_fc_ctrl *ctrl)
 
 	memset(sqe, 0, sizeof(*sqe));
 	sqe->common.opcode = nvme_admin_async_event;
-	sqe->common.command_id = AEN_CMDID_BASE;
+	sqe->common.command_id = NVME_AQ_BLKMQ_DEPTH;
 
 	return 0;
 }

@@ -2025,7 +2016,7 @@ nvme_fc_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 	unsigned long flags;
 	bool terminating = false;
 
-	if (aer_idx > NVME_FC_NR_AEN_COMMANDS)
+	if (aer_idx > NVME_NR_AERS)
 		return;
 
 	spin_lock_irqsave(&ctrl->lock, flags);

@@ -2689,7 +2680,7 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
 	ctrl->admin_tag_set.ops = &nvme_fc_admin_mq_ops;
-	ctrl->admin_tag_set.queue_depth = NVME_FC_AQ_BLKMQ_DEPTH - 1;
+	ctrl->admin_tag_set.queue_depth = NVME_AQ_BLKMQ_DEPTH - 1;
 	ctrl->admin_tag_set.reserved_tags = 2; /* fabric connect + Keep-Alive */
 	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_fc_fcp_op) +

drivers/nvme/host/nvme.h +0 -1

index 8f2a168..0510a33 100644
@@ -291,7 +291,6 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 		bool send);
 
-#define NVME_NR_AERS	1
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res);
 void nvme_queue_async_events(struct nvme_ctrl *ctrl);

drivers/nvme/host/pci.c +0 -6

index 882ed36..6bd49b2 100644
@@ -39,12 +39,6 @@
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
 
-/*
- * We handle AEN commands ourselves and don't even let the
- * block layer know about them.
- */
-#define NVME_AQ_BLKMQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AERS)
-
 static int use_threaded_interrupts;
 module_param(use_threaded_interrupts, int, 0);

drivers/nvme/host/rdma.c +3 ~~-11~~

index da04df1..8d811f4 100644
@@ -42,14 +42,6 @@
 
 #define NVME_RDMA_MAX_INLINE_SEGMENTS	1
 
-/*
- * We handle AEN commands ourselves and don't even let the
- * block layer know about them.
- */
-#define NVME_RDMA_NR_AEN_COMMANDS      1
-#define NVME_RDMA_AQ_BLKMQ_DEPTH       \
-	(NVME_AQ_DEPTH - NVME_RDMA_NR_AEN_COMMANDS)
-
 struct nvme_rdma_device {
 	struct ib_device       *dev;
 	struct ib_pd	       *pd;

@@ -1116,7 +1108,7 @@ static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 
 	memset(cmd, 0, sizeof(*cmd));
 	cmd->common.opcode = nvme_admin_async_event;
-	cmd->common.command_id = NVME_RDMA_AQ_BLKMQ_DEPTH;
+	cmd->common.command_id = NVME_AQ_BLKMQ_DEPTH;
 	cmd->common.flags |= NVME_CMD_SGL_METABUF;
 	nvme_rdma_set_sg_null(cmd);

@@ -1178,7 +1170,7 @@ static int __nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc, int tag)
 	 * for them but rather special case them here.
 	 */
 	if (unlikely(nvme_rdma_queue_idx(queue) == 0 &&
-			cqe->command_id >= NVME_RDMA_AQ_BLKMQ_DEPTH))
+			cqe->command_id == NVME_AQ_BLKMQ_DEPTH))
 		nvme_complete_async_event(&queue->ctrl->ctrl, cqe->status,
 				&cqe->result);
 	else

@@ -1542,7 +1534,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl)
 
 	memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
 	ctrl->admin_tag_set.ops = &nvme_rdma_admin_mq_ops;
-	ctrl->admin_tag_set.queue_depth = NVME_RDMA_AQ_BLKMQ_DEPTH;
+	ctrl->admin_tag_set.queue_depth = NVME_AQ_BLKMQ_DEPTH - 1;
 	ctrl->admin_tag_set.reserved_tags = 2; /* connect + keep-alive */
 	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_rdma_request) +

drivers/nvme/target/loop.c +3 ~~-11~~

index 717ed7d..336e46e 100644
@@ -23,14 +23,6 @@
 
 #define NVME_LOOP_MAX_SEGMENTS		256
 
-/*
- * We handle AEN commands ourselves and don't even let the
- * block layer know about them.
- */
-#define NVME_LOOP_NR_AEN_COMMANDS	1
-#define NVME_LOOP_AQ_BLKMQ_DEPTH	\
-	(NVME_AQ_DEPTH - NVME_LOOP_NR_AEN_COMMANDS)
-
 struct nvme_loop_iod {
 	struct nvme_request	nvme_req;
 	struct nvme_command	cmd;

@@ -113,7 +105,7 @@ static void nvme_loop_queue_response(struct nvmet_req *req)
 	 * for them but rather special case them here.
 	 */
 	if (unlikely(nvme_loop_queue_idx(queue) == 0 &&
-			cqe->command_id >= NVME_LOOP_AQ_BLKMQ_DEPTH)) {
+			cqe->command_id >= NVME_AQ_BLKMQ_DEPTH)) {
 		nvme_complete_async_event(&queue->ctrl->ctrl, cqe->status,
 				&cqe->result);
 	} else {

@@ -201,7 +193,7 @@ static void nvme_loop_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 
 	memset(&iod->cmd, 0, sizeof(iod->cmd));
 	iod->cmd.common.opcode = nvme_admin_async_event;
-	iod->cmd.common.command_id = NVME_LOOP_AQ_BLKMQ_DEPTH;
+	iod->cmd.common.command_id = NVME_AQ_BLKMQ_DEPTH;
 	iod->cmd.common.flags |= NVME_CMD_SGL_METABUF;
 
 	if (!nvmet_req_init(&iod->req, &queue->nvme_cq, &queue->nvme_sq,

@@ -357,7 +349,7 @@ static int nvme_loop_configure_admin_queue(struct nvme_loop_ctrl *ctrl)
 
 	memset(&ctrl->admin_tag_set, 0, sizeof(ctrl->admin_tag_set));
 	ctrl->admin_tag_set.ops = &nvme_loop_admin_mq_ops;
-	ctrl->admin_tag_set.queue_depth = NVME_LOOP_AQ_BLKMQ_DEPTH;
+	ctrl->admin_tag_set.queue_depth = NVME_AQ_BLKMQ_DEPTH - 1;
 	ctrl->admin_tag_set.reserved_tags = 2; /* connect + keep-alive */
 	ctrl->admin_tag_set.numa_node = NUMA_NO_NODE;
 	ctrl->admin_tag_set.cmd_size = sizeof(struct nvme_loop_iod) +

include/linux/nvme.h +7 -0

index 6b8ee9e..a253977 100644
@@ -89,6 +89,13 @@ enum {
 
 #define NVME_AQ_DEPTH		32
 
+/*
+ * We handle AEN commands ourselves and don't even let the
+ * block layer know about them.
+ */
+#define NVME_NR_AERS	1
+#define NVME_AQ_BLKMQ_DEPTH	(NVME_AQ_DEPTH - NVME_NR_AERS)
+
 enum {
 	NVME_REG_CAP	= 0x0000,	/* Controller Capabilities */
 	NVME_REG_VS	= 0x0008,	/* Version */

Signature · 1 line

2.5.5

Sagi Grimberg

maintainer

2017-07-10 06:01 UTC #12 lore raw

PATCH 4/7 nvme: Centralize blk-mq tag and AEN counts

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

James Smart

2017-07-10 23:51 UTC #13 lore raw

PATCH 4/7 nvme: Centralize blk-mq tag and AEN counts

In reply to Keith Busch

Quoted 6 lines · Keith Busch

All the drivers were duplicating AEN accounting, yet they all depend on the core to use the exact same settings of a single AEN to be deducted from the tagset depth. This patch moves admin queue command accounting to the core.

Looks good.

Reviewed-by James Smart <james.smart at broadcom.com>

Keith Busch

author

2017-07-07 16:22 UTC #14 lore raw

PATCH 5/7 nvme: Only one AEN request

In reply to Keith Busch

This driver issues only one AEN request, so all the logic to handle multiple ones is unnecessary. Even if we did allow multiple ones, the implementation is broken as it will create command id collisions. For those two reasons, this patch deletes all the aer index usage, assuming there's only one.

As a consequence of removing the event limit, the core's callback for these events is simplified to only handle the successful case. The "abort" status handling for AER has been broken for some time anyway:
it was meant to reclaim the aer count from a driver cancelled command after a reset, but that was relying on blk-mq to manage the reserved tags. We don't use blk-mq tags for AER, though, so we will never see a synthesized abort status.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c   | 27 +++------------------------
 drivers/nvme/host/fc.c     |  5 +----
 drivers/nvme/host/nvme.h   |  3 +--
 drivers/nvme/host/pci.c    |  4 ++--
 drivers/nvme/host/rdma.c   |  5 +----
 drivers/nvme/target/loop.c |  2 +-
 6 files changed, 9 insertions(+), 37 deletions(-)

drivers/nvme/host/core.c +3 ~~-24~~

index cb96f4a..bfd0045 100644
@@ -2512,36 +2512,15 @@ static void nvme_async_event_work(struct work_struct *work)
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
 
-	spin_lock_irq(&ctrl->lock);
-	while (ctrl->event_limit > 0) {
-		int aer_idx = --ctrl->event_limit;
-
-		spin_unlock_irq(&ctrl->lock);
-		ctrl->ops->submit_async_event(ctrl, aer_idx);
-		spin_lock_irq(&ctrl->lock);
-	}
-	spin_unlock_irq(&ctrl->lock);
+	ctrl->ops->submit_async_event(ctrl);
 }
 
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res)
 {
 	u32 result = le32_to_cpu(res->u32);
-	bool done = true;
 
-	switch (le16_to_cpu(status) >> 1) {
-	case NVME_SC_SUCCESS:
-		done = false;
-		/*FALLTHRU*/
-	case NVME_SC_ABORT_REQ:
-		++ctrl->event_limit;
-		queue_work(nvme_wq, &ctrl->async_event_work);
-		break;
-	default:
-		break;
-	}
-
-	if (done)
+	if ((le16_to_cpu(status) >> 1) != NVME_SC_SUCCESS)
 		return;
 
 	switch (result & 0xff07) {

@@ -2552,12 +2531,12 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
+	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);
 
 void nvme_queue_async_events(struct nvme_ctrl *ctrl)
 {
-	ctrl->event_limit = NVME_NR_AERS;
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 EXPORT_SYMBOL_GPL(nvme_queue_async_events);

drivers/nvme/host/fc.c +1 -4

index 753761c..f1ac8c0 100644
@@ -2010,15 +2010,12 @@ nvme_fc_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
 }
 
 static void
-nvme_fc_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
+nvme_fc_submit_async_event(struct nvme_ctrl *arg)
 {
 	struct nvme_fc_ctrl *ctrl = to_fc_ctrl(arg);
 	unsigned long flags;
 	bool terminating = false;
 
-	if (aer_idx > NVME_NR_AERS)
-		return;
-
 	spin_lock_irqsave(&ctrl->lock, flags);
 	if (ctrl->flags & FCCTRL_TERMIO)
 		terminating = true;

drivers/nvme/host/nvme.h +1 -2

index 0510a33..edca92d 100644
@@ -153,7 +153,6 @@ struct nvme_ctrl {
 	u16 nssa;
 	u16 nr_streams;
 	atomic_t abort_limit;
-	u8 event_limit;
 	u8 vwc;
 	u32 vs;
 	u32 sgls;

@@ -226,7 +225,7 @@ struct nvme_ctrl_ops {
 	int (*reg_write32)(struct nvme_ctrl *ctrl, u32 off, u32 val);
 	int (*reg_read64)(struct nvme_ctrl *ctrl, u32 off, u64 *val);
 	void (*free_ctrl)(struct nvme_ctrl *ctrl);
-	void (*submit_async_event)(struct nvme_ctrl *ctrl, int aer_idx);
+	void (*submit_async_event)(struct nvme_ctrl *ctrl);
 	int (*delete_ctrl)(struct nvme_ctrl *ctrl);
 	int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
 };

drivers/nvme/host/pci.c +2 -2

index 6bd49b2..a4e3261 100644
@@ -846,7 +846,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
 	return __nvme_poll(nvmeq, tag);
 }
 
-static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl, int aer_idx)
+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];

@@ -854,7 +854,7 @@ static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl, int aer_idx)
 
 	memset(&c, 0, sizeof(c));
 	c.common.opcode = nvme_admin_async_event;
-	c.common.command_id = NVME_AQ_BLKMQ_DEPTH + aer_idx;
+	c.common.command_id = NVME_AQ_BLKMQ_DEPTH;
 
 	spin_lock_irq(&nvmeq->q_lock);
 	__nvme_submit_cmd(nvmeq, &c);

drivers/nvme/host/rdma.c +1 -4

index 8d811f4..f739540 100644
@@ -1091,7 +1091,7 @@ static struct blk_mq_tags *nvme_rdma_tagset(struct nvme_rdma_queue *queue)
 	return queue->ctrl->tag_set.tags[queue_idx - 1];
 }
 
-static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
+static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(arg);
 	struct nvme_rdma_queue *queue = &ctrl->queues[0];

@@ -1101,9 +1101,6 @@ static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
 	struct ib_sge sge;
 	int ret;
 
-	if (WARN_ON_ONCE(aer_idx != 0))
-		return;
-
 	ib_dma_sync_single_for_cpu(dev, sqe->dma, sizeof(*cmd), DMA_TO_DEVICE);
 
 	memset(cmd, 0, sizeof(*cmd));

drivers/nvme/target/loop.c +1 -1

index 336e46e..af0d188 100644
@@ -185,7 +185,7 @@ static blk_status_t nvme_loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_OK;
 }
 
-static void nvme_loop_submit_async_event(struct nvme_ctrl *arg, int aer_idx)
+static void nvme_loop_submit_async_event(struct nvme_ctrl *arg)
 {
 	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(arg);
 	struct nvme_loop_queue *queue = &ctrl->queues[0];

Signature · 1 line

2.5.5

Sagi Grimberg

maintainer

2017-07-10 06:06 UTC #15 lore raw

PATCH 5/7 nvme: Only one AEN request

Looks good,

Does this apply on my patch?
[PATCH v3] nvme: split nvme_uninit_ctrl into stop and uninit

I'll update nvme-4.13

James Smart

2017-07-10 23:53 UTC #16 lore raw

PATCH 5/7 nvme: Only one AEN request

In reply to Keith Busch

Quoted 6 lines · Keith Busch

This driver issues only one AEN request, so all the logic to handle multiple ones is unnecessary. Even if we did allow multiple ones, the implementation is broken as it will create command id collisions. For those two reasons, this patch deletes all the aer index usage, assuming there's only one.

Quoted 8 lines

As a consequence of removing the event limit, the core's callback for these events is simplified to only handle the successful case. The "abort" status handling for AER has been broken for some time anyway:
it was meant to reclaim the aer count from a driver cancelled command after a reset, but that was relying on blk-mq to manage the reserved tags. We don't use blk-mq tags for AER, though, so we will never see a synthesized abort status.

Looks good

Reviewed-by James Smart <james.smart at broadcom.com>

Keith Busch

author

2017-07-07 16:23 UTC #17 lore raw

PATCH 6/7 nvme: Unexport starting async event work

In reply to Keith Busch

Async event work is for core use only and should not be called directly from drivers.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c | 3 +--
 drivers/nvme/host/nvme.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

drivers/nvme/host/core.c +1 -2

index bfd0045..783db84 100644
@@ -2535,11 +2535,10 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);
 
-void nvme_queue_async_events(struct nvme_ctrl *ctrl)
+static void nvme_queue_async_events(struct nvme_ctrl *ctrl)
 {
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
-EXPORT_SYMBOL_GPL(nvme_queue_async_events);
 
 static DEFINE_IDA(nvme_instance_ida);

drivers/nvme/host/nvme.h +0 -1

index edca92d..b65c22f 100644
@@ -292,7 +292,6 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res);
-void nvme_queue_async_events(struct nvme_ctrl *ctrl);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);

Signature · 1 line

2.5.5

Christoph Hellwig

maintainer

2017-07-07 18:11 UTC #18 lore raw

PATCH 6/7 nvme: Unexport starting async event work

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2542

Quoted 5 lines

drivers/nvme/host/core.c +1 -1

+static void nvme_queue_async_events(struct nvme_ctrl *ctrl)
 {
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
-EXPORT_SYMBOL_GPL(nvme_queue_async_events);

I'd just kill this helper now that it's not even exported.

Keith Busch

author

2017-07-07 19:54 UTC #19 lore raw

PATCH 6/7 nvme: Unexport starting async event work

Quoted 7 lines · Christoph Hellwig

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2542
Quoted 5 lines
drivers/nvme/host/core.c +1 -1
+static void nvme_queue_async_events(struct nvme_ctrl *ctrl)
 {
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
-EXPORT_SYMBOL_GPL(nvme_queue_async_events);
I'd just kill this helper now that it's not even exported.

Agreed, I'll take that out for v2. I'll wait till next week to see if there's any comments on the previous patches before sending.

Keith Busch

author

2017-07-07 16:23 UTC #20 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

In reply to Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send. This implementation saves the last AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.

The udev rule used to test this was the following:

  ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="*", \
        RUN+="/bin/sh -c '/usr/local/sbin/nvme get-log $env{DEVNAME} --log-id=$(( ($env{NVME_AEN} >> 16) & 0xff )) --log-len=4096 >> /tmp/nvme-log'"

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c | 16 ++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 17 insertions(+)

drivers/nvme/host/core.c +16 -0

index 783db84..4abde2da 100644
@@ -2507,11 +2507,26 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
+void nvme_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	char buffer[20]; /* NVME_AEN=0xffffffff\0 */
+	char *envp[2] = {buffer, NULL};
+	u32 aen = ctrl->aen;
+
+	ctrl->aen = 0;
+	if (!aen)
+		return;
+
+	snprintf(buffer, sizeof(buffer), "NVME_AEN=%#08x", aen);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
 static void nvme_async_event_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
 
+	nvme_aen_uevent(ctrl);
 	ctrl->ops->submit_async_event(ctrl);
 }

@@ -2531,6 +2546,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
+	ctrl->aen = result;
 	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);

drivers/nvme/host/nvme.h +1 -0

index b65c22f..898815f 100644
@@ -159,6 +159,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	u32 aen;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;

Signature · 1 line

2.5.5

Christoph Hellwig

maintainer

2017-07-07 18:14 UTC #21 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 11 lines · Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send. This implementation saves the last AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.
The udev rule used to test this was the following:
  ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="*", \
        RUN+="/bin/sh -c '/usr/local/sbin/nvme get-log $env{DEVNAME} --log-id=$(( ($env{NVME_AEN} >> 16) & 0xff )) --log-len=4096 >> /tmp/nvme-log'"

Can we have an nvme-aen helper that gets invoked by a trivial udev rule, which would read a config file for policy?

Also I think we need to document our policy on which AERs get forwarded to userspace. There are some we really should be handling in the kernel (fw activation which is in progress, namespace data changes really needs fixing, and ANA will also require kernel handling). Is there a way to state it's up to the kernel to reserve any even for itself? Or should we just forward very specific AERs to userspace?

Keith Busch

author

2017-07-13 21:29 UTC #22 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 3 lines · Christoph Hellwig

Can we have an nvme-aen helper that gets invoked by a trivial udev rule, which would read a config file for policy?

Sure, I can work on an aen helper for udev. Do you know if there is an existing project for these sorts of things that would take this feature?

Quoted 7 lines

Also I think we need to document our policy on which AERs get forwarded to userspace. There are some we really should be handling in the kernel (fw activation which is in progress, namespace data changes really needs fixing, and ANA will also require kernel handling). Is there a way to state it's up to the kernel to reserve any even for itself? Or should we just forward very specific AERs to userspace?

I was hoping to not have a policy in the kernel. If we filter them with evolving black or white lists, user space would still need to account for difference across kernel versions.

Instead, sending everything is a lot more straight forward policy, and user space can decide what to do with it. It's future proof to any new event and use case. Even if the driver handles the event, there shouldn't be anything user space can do to harm it. In the worst case, it's just giving information that udev doesn't need.

Guilherme G. Piccoli

2017-07-14 12:50 UTC #23 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 7 lines · Keith Busch

Quoted 3 lines · Christoph Hellwig
Can we have an nvme-aen helper that gets invoked by a trivial udev rule, which would read a config file for policy?
Sure, I can work on an aen helper for udev. Do you know if there is an existing project for these sorts of things that would take this feature?

We were planning to implement this here on LTC, and some ideas in discussion is whether such tool could be integrated to nvme-cli, like a "nvmed" tool, a daemon to handle the events. Advantages of the daemon are possible implementation of RAS features like trigger actions based on adapter's error, etc.

Do you think this kind of tool is interesting? Suggestions on this topic are greatly appreciated. One of our concerns is that AEN events still need to be enabled by the user, not all events are enabled by default. One such tool would allow friendly configuration on which events user wants to enable.

Thanks,

Guilherme

Christoph Hellwig

maintainer

2017-07-14 12:54 UTC #24 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 5 lines · Guilherme G. Piccoli

We were planning to implement this here on LTC, and some ideas in discussion is whether such tool could be integrated to nvme-cli, like a "nvmed" tool, a daemon to handle the events. Advantages of the daemon are possible implementation of RAS features like trigger actions based on adapter's error, etc.

Don't uevent call a binary each time? Either way nvme-cli is the right place for it.

Quoted 5 lines

Do you think this kind of tool is interesting? Suggestions on this topic are greatly appreciated. One of our concerns is that AEN events still need to be enabled by the user, not all events are enabled by default. One such tool would allow friendly configuration on which events user wants to enable.

Yes, AERs generally are opt-in.

Christoph Hellwig

maintainer

2017-07-14 12:53 UTC #25 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

In reply to Keith Busch

Quoted 7 lines · Keith Busch

Quoted 3 lines · Christoph Hellwig
Can we have an nvme-aen helper that gets invoked by a trivial udev rule, which would read a config file for policy?
Sure, I can work on an aen helper for udev. Do you know if there is an existing project for these sorts of things that would take this feature?

I would have expected this to be part of nvme-cli in one form or another.

Quoted 9 lines

I was hoping to not have a policy in the kernel. If we filter them with evolving black or white lists, user space would still need to account for difference across kernel versions.
Instead, sending everything is a lot more straight forward policy, and user space can decide what to do with it. It's future proof to any new event and use case. Even if the driver handles the event, there shouldn't be anything user space can do to harm it. In the worst case, it's just giving information that udev doesn't need.

Someone has to clear the log page after and AER, and if both the kernel and the user tool do so we risk that the user tool clears the next AER due to a race condition.

But maybe we can work around this by adding a bit to the event that indicates if the kernel has cleared the event.

Keith Busch

author

2017-07-14 15:16 UTC #26 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 4 lines · Christoph Hellwig

Someone has to clear the log page after and AER, and if both the kernel and the user tool do so we risk that the user tool clears the next AER due to a race condition.

Sorry, bare with me a moment. I may be missing something obvious, as I'm not seeing a problem. Reading the event's associated log only rearms the AER for that event type. Even if the user tool reads the same log as the driver, that doesn't stop the driver from seeing the next AER completion and handle it the same as if user space wasn't there.

Christoph Hellwig

maintainer

2017-07-15 08:42 UTC #27 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 11 lines · Keith Busch

Quoted 4 lines · Christoph Hellwig
Someone has to clear the log page after and AER, and if both the kernel and the user tool do so we risk that the user tool clears the next AER due to a race condition.
Sorry, bare with me a moment. I may be missing something obvious, as I'm not seeing a problem. Reading the event's associated log only rearms the AER for that event type. Even if the user tool reads the same log as the driver, that doesn't stop the driver from seeing the next AER completion and handle it the same as if user space wasn't there.

Take the Changed Namespace List log page for example.

We get an AER, and then both schedule a log page read in the kernel (as we should, patch pending) and send the uevent. As soon as the read the log page in the kernel to clear the log page we get another event due to another namespace change, but not userspace manages to read the log page before the kernel acts on the next AER, so the kernel won't see the changes.

And yes, this is probably a little constructed, but we'll get more log pages with similar semantics, so I am worried a bit about this.

Keith Busch

author

2017-07-17 15:22 UTC #28 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 10 lines · Christoph Hellwig

Take the Changed Namespace List log page for example.
We get an AER, and then both schedule a log page read in the kernel (as we should, patch pending) and send the uevent. As soon as the read the log page in the kernel to clear the log page we get another event due to another namespace change, but not userspace manages to read the log page before the kernel acts on the next AER, so the kernel won't see the changes.

Quoted 2 lines

And yes, this is probably a little constructed, but we'll get more log pages with similar semantics, so I am worried a bit about this.

That event needs to already be handled by the driver without relying on the Changed Namespace List log, like when the list exceeds the log size. But okay, I see the concern in general if reading a log page may change the results.

Guan Junxiong

2017-08-01 11:59 UTC #29 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Hi, Keith

Any update about this patch? We are keen on planning to use this kind of feature to handle some events of controller/subsystem with multipath-tools.

Thanks

Best wishes
Guan Junxiong

Quoted 25 lines · Keith Busch

Quoted 10 lines · Christoph Hellwig
Take the Changed Namespace List log page for example.
We get an AER, and then both schedule a log page read in the kernel (as we should, patch pending) and send the uevent. As soon as the read the log page in the kernel to clear the log page we get another event due to another namespace change, but not userspace manages to read the log page before the kernel acts on the next AER, so the kernel won't see the changes.
Quoted 2 lines
And yes, this is probably a little constructed, but we'll get more log pages with similar semantics, so I am worried a bit about this.
That event needs to already be handled by the driver without relying on the Changed Namespace List log, like when the list exceeds the log size. But okay, I see the concern in general if reading a log page may change the results.
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Zou Ming

2017-07-10 10:17 UTC #30 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

In reply to Keith Busch

Hi Keith:
Thanks,Send All AEN info to user space looks good to me.

In addition I have a question, is there a scene where repeated asynchronous events are received?
Because the event is sent to the user space for asynchronous processing, and at the same time issued a new asynchronous event request command. The user space may be processed slower than the issued command, resulting in a duplicate asynchronous event report.

Best regards,
Zou Ming.

Quoted 6 lines · Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send. This implementation saves the last AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.

Keith Busch

author

2017-07-10 17:38 UTC #31 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 6 lines · Zou Ming

Hi Keith:
Thanks,Send All AEN info to user space looks good to me.
In addition I have a question, is there a scene where repeated asynchronous events are received?
Because the event is sent to the user space for asynchronous processing, and at the same time issued a new asynchronous event request command. The user space may be processed slower than the issued command, resulting in a duplicate asynchronous event report.

The driver can have only one async event request outstanding, so it can't have more than one uevent available to send. The driver will send the KOBJ_CHANGE prior to sending the next AEN request, so that should be sufficient if anything is monitoring the events.

James Smart

2017-07-10 23:57 UTC #32 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

In reply to Keith Busch

Quoted 30 lines · Keith Busch

This will give udev a chance to handle asynchronous event notification and clear the log to unmask future events of the same type. Since the core driver allows only one AEN request at a time, we can only have one possible oustanding uevent to send. This implementation saves the last AEN result from the IRQ handler, and sends the uevent change notification when the AEN work is rescheduled.
The udev rule used to test this was the following:
   ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_AEN}=="*", \
         RUN+="/bin/sh -c '/usr/local/sbin/nvme get-log $env{DEVNAME} --log-id=$(( ($env{NVME_AEN} >> 16) & 0xff )) --log-len=4096 >> /tmp/nvme-log'"
Signed-off-by Keith Busch <keith.busch at intel.com>
---
  drivers/nvme/host/core.c | 16 ++++++++++++++++
  drivers/nvme/host/nvme.h |  1 +
  2 files changed, 17 insertions(+)
drivers/nvme/host/core.c +4 -0
index 783db84..4abde2da 100644
@@ -2507,11 +2507,26 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
  }
  EXPORT_SYMBOL_GPL(nvme_remove_namespaces);
 
+void nvme_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	char buffer[20]; /* NVME_AEN=0xffffffff\0 */
+	char *envp[2] = {buffer, NULL};

drivers/nvme/host/core.c void nvme_remove_namespaces(struct nvme_ctrl *ctrl) · line 2523

Quoted 10 lines

drivers/nvme/host/core.c +10 -0

+	u32 aen = ctrl->aen;
+
+	ctrl->aen = 0;
+	if (!aen)
+		return;
+
+	snprintf(buffer, sizeof(buffer), "NVME_AEN=%#08x", aen);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+

Passing on a comment from Christoph received on a similar fc udev event:
"Please use kasprintf so that we have a dynamic allocation and don't need to hardcode buffer sizes"

-- james

Keith Busch

author

2017-07-11 14:43 UTC #33 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 11 lines · James Smart

Quoted 10 lines · Keith Busch

drivers/nvme/host/core.c +10 -0

+void nvme_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	char buffer[20]; /* NVME_AEN=0xffffffff\0 */
+	char *envp[2] = {buffer, NULL};
+	u32 aen = ctrl->aen;
+
+	ctrl->aen = 0;
+	if (!aen)
+		return;
+

Quoted 8 lines

drivers/nvme/host/core.c void nvme_remove_namespaces(struct nvme_ctrl *ctrl) · line 2523
Quoted 4 lines
drivers/nvme/host/core.c +4 -0
+	snprintf(buffer, sizeof(buffer), "NVME_AEN=%#08x", aen);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
Passing on a comment from Christoph received on a similar fc udev event:
"Please use kasprintf so that we have a dynamic allocation and don't need to hardcode buffer sizes"

I agree that usage for dynamic strings is better, but this is a fixed sized string that will always be 20 bytes.

James Smart

2017-07-11 17:14 UTC #34 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 19 lines · Keith Busch

Quoted 10 lines · James Smart
Quoted 9 lines · Keith Busch
drivers/nvme/host/core.c +9 -0
+void nvme_aen_uevent(struct nvme_ctrl *ctrl)
+{
+	char buffer[20]; /* NVME_AEN=0xffffffff\0 */
+	char *envp[2] = {buffer, NULL};
+	u32 aen = ctrl->aen;
+
+	ctrl->aen = 0;
+	if (!aen)
+		return;
Quoted 8 lines
drivers/nvme/host/core.c void nvme_remove_namespaces(struct nvme_ctrl *ctrl) · line 2523
Quoted 5 lines
drivers/nvme/host/core.c +5 -0
+
+	snprintf(buffer, sizeof(buffer), "NVME_AEN=%#08x", aen);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
Passing on a comment from Christoph received on a similar fc udev event:
"Please use kasprintf so that we have a dynamic allocation and don't need to hardcode buffer sizes"

Quoted 2 lines

I agree that usage for dynamic strings is better, but this is a fixed sized string that will always be 20 bytes.

My scenario was the same - fixed sized, not dynamic. Just being the messenger for consistency.

-- james

Keith Busch

author

2017-07-13 16:09 UTC #35 lore raw

PATCH 7/7 nvme: Send change uevent when AEN completes

Quoted 6 lines · James Smart

Quoted 2 lines · Keith Busch
I agree that usage for dynamic strings is better, but this is a fixed sized string that will always be 20 bytes.
My scenario was the same - fixed sized, not dynamic. Just being the messenger for consistency.

Wha?! That style point is not one I'm convinced is better. Not only is it more complicated, the stack overhead of calling kasprintf is higher than the 20 bytes I'm using here.

Christoph Hellwig

maintainer

2017-07-07 18:09 UTC #36 lore raw

cover NVMe Async Event Notification updates

In reply to Keith Busch

I do like the code consolidation. I'll throw in a few comments while at the airport, a real review will follow.