Skip to contentSkip to conversation

nvme: Submit uevents for log page notification

RFC PATCH New Keith Busch 4 participants 5 messages Mar 24, 2017 – Jun 15, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 nvme: Submit uevents for log page notification

This is a first attempt at adding uevents to nvme. The concept was discussed at LSFMM, so here it is for consideration. :)

In this implementation, the driver will submit a "change" uevent whenever the controller indicates a log page contains pertinent information. This happens in response to an Asynchronouse Event Notification, or if a command completes with the "MORE" status bit set. If there are other events anyone thinks we'd like udev to get a chance to handle, or would prefer to see these variables submitted to udev in a different format, please let me know.

Submitting a uevent from the kernel can't be done from an irq context, which is the context the driver learns of such event, so this path enqueues the log identifier of internest on a FIFO, then has the async event work flush the event FIFO to udev. Pretty simple.

Tested with the following rule to kick an nvme-cli generic get-log to clear the log request and append the returned log data to a temporary log. This is just an example for testing and not intended for real life use.

ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_LOG}=="*", \
	RUN+="/bin/sh -c '/usr/local/sbin/nvme get-log $env{DEVNAME} --log-id=$env{NVME_LOG} --log-len=4096 >> /tmp/nvme-log'"
Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c   | 35 +++++++++++++++++++++++++++++++++--
 drivers/nvme/host/fc.c     |  2 ++
 drivers/nvme/host/nvme.h   |  4 ++++
 drivers/nvme/host/pci.c    |  2 ++
 drivers/nvme/host/rdma.c   |  2 ++
 drivers/nvme/target/loop.c |  2 ++
 include/linux/nvme.h       |  2 ++
 7 files changed, 47 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +33 -2
index 9b3b57f..a757deb 100644
@@ -1879,6 +1879,27 @@ static const struct attribute_group *nvme_dev_attr_groups[] = {
 	NULL,
 };
 
+void nvme_uevent_work(struct nvme_ctrl *ctrl, int log)
+{
+	char buffer[13]; /* NVME_LOG=255\0 */
+	char *envp[2] = {buffer, NULL};
+
+	snprintf(buffer, sizeof(buffer), "NVME_LOG=%d", log);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+}
+
+void nvme_uevent(struct nvme_ctrl *ctrl, int log_page)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+	kfifo_put(&ctrl->log_event_fifo, log_page);
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	schedule_work(&ctrl->async_event_work);
+}
+EXPORT_SYMBOL_GPL(nvme_uevent);
+
 static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 {
 	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
@@ -2149,8 +2170,14 @@ static void nvme_async_event_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, async_event_work);
+	int log_page;
 
 	spin_lock_irq(&ctrl->lock);
+	while (kfifo_get(&ctrl->log_event_fifo, &log_page)) {
+		spin_unlock_irq(&ctrl->lock);
+		nvme_uevent_work(ctrl, log_page);
+		spin_lock_irq(&ctrl->lock);
+	}
 	while (ctrl->event_limit > 0) {
 		int aer_idx = --ctrl->event_limit;
 
@@ -2165,6 +2192,8 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res)
 {
 	u32 result = le32_to_cpu(res->u32);
+	u8 log_page = (result >> 16) & 8;
+	u8 event_type = result & 7;
 	bool done = true;
 
 	switch (le16_to_cpu(status) >> 1) {
@@ -2173,7 +2202,6 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		/*FALLTHRU*/
 	case NVME_SC_ABORT_REQ:
 		++ctrl->event_limit;
-		schedule_work(&ctrl->async_event_work);
 		break;
 	default:
 		break;
@@ -2182,13 +2210,15 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	if (done)
 		return;
 
-	switch (result & 0xff07) {
+	nvme_uevent(ctrl, log_page);
+	switch (event_type) {
 	case NVME_AER_NOTICE_NS_CHANGED:
 		dev_info(ctrl->device, "rescanning\n");
 		nvme_queue_scan(ctrl);
 		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
+		break;
 	}
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);
@@ -2280,6 +2310,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->quirks = quirks;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
 	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
+	INIT_KFIFO(ctrl->log_event_fifo);
 
 	ret = nvme_set_instance(ctrl);
 	if (ret)
drivers/nvme/host/fc.c +2 -0
index 9690beb..3930a12 100644
@@ -1936,6 +1936,8 @@ nvme_fc_complete_rq(struct request *rq)
 			nvme_requeue_req(rq);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(rq))
 			error = rq->errors;
drivers/nvme/host/nvme.h +4 -0
index 2aa20e3..fda6ebb 100644
@@ -173,6 +173,9 @@ struct nvme_ctrl {
 	u16 icdoff;
 	u16 maxcmd;
 	struct nvmf_ctrl_options *opts;
+
+#define NVME_EVENT_FIFO_SIZE 8
+	DECLARE_KFIFO(log_event_fifo, int, NVME_EVENT_FIFO_SIZE);
 };
 
 /*
@@ -290,6 +293,7 @@ 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_uevent(struct nvme_ctrl *ctrl, int log_id);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
drivers/nvme/host/pci.c +2 -0
index 26a5fd0..6a2f0d3 100644
@@ -642,6 +642,8 @@ static void nvme_complete_rq(struct request *req)
 			nvme_requeue_req(req);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(req))
 			error = req->errors;
drivers/nvme/host/rdma.c +2 -0
index 779f516..d8397665 100644
@@ -1507,6 +1507,8 @@ static void nvme_rdma_complete_rq(struct request *rq)
 			nvme_requeue_req(rq);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(rq))
 			error = rq->errors;
drivers/nvme/target/loop.c +2 -0
index d1f06e7..369b5de3 100644
@@ -103,6 +103,8 @@ static void nvme_loop_complete_rq(struct request *req)
 			nvme_requeue_req(req);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(req))
 			error = req->errors;
include/linux/nvme.h +2 -0
index c43d435..e711de6 100644
@@ -16,6 +16,7 @@
 #define _LINUX_NVME_H
 
 #include <linux/types.h>
+#include <linux/kfifo.h>
 
 /* NQN names in commands fields specified one size */
 #define NVMF_NQN_FIELD_LEN	256
@@ -1003,6 +1004,7 @@ enum {
 	NVME_SC_ACCESS_DENIED		= 0x286,
 	NVME_SC_UNWRITTEN_BLOCK		= 0x287,
 
+	NVME_SC_MORE			= 0x2000,
 	NVME_SC_DNR			= 0x4000,
 
 
Signature · 1 line
2.7.2
#1 lore raw

RFC PATCH 1/1 nvme: Submit uevents for log page notification

The uevent itself looks fine to me, but we really need to figure out how we can clear log pages in kernel space where needed.

We should already be doing this for the namespace changed one, and it will be very important at least for ANA as well.

A few more mechanical code comments below:

Quoted 1 line

+void nvme_uevent_work(struct nvme_ctrl *ctrl, int log)

static?

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

Quoted 12 lines

{

drivers/nvme/host/core.c +2 -1
 	u32 result = le32_to_cpu(res->u32);
+	u8 log_page = (result >> 16) & 8;
+	u8 event_type = result & 7;
 	bool done = true;
 
 	switch (le16_to_cpu(status) >> 1) {
@@ -2173,7 +2202,6 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		/*FALLTHRU*/
 	case NVME_SC_ABORT_REQ:
 		++ctrl->event_limit;
-		schedule_work(&ctrl->async_event_work);

This seems to disable any action on AERs in the kernel..

drivers/nvme/host/fc.c nvme_fc_complete_rq(struct request *rq) · line 1943

Quoted 13 lines
drivers/nvme/host/fc.c +2 -0
index 9690beb..3930a12 100644
@@ -1936,6 +1936,8 @@ nvme_fc_complete_rq(struct request *rq)
 			nvme_requeue_req(rq);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(rq))
 			error = rq->errors;

We'll really need to factor the common request completion code into a helper in common code first..

include/linux/nvme.h line 19

Quoted 9 lines
include/linux/nvme.h +1 -0
index c43d435..e711de6 100644
@@ -16,6 +16,7 @@
 #define _LINUX_NVME_H
 
 #include <linux/types.h>
+#include <linux/kfifo.h>

This should move to drivers/nvme/host/nvme.h instead.

#2 lore raw

RFC PATCH 1/1 nvme: Submit uevents for log page notification

Given that we have the uevent discussion again can you resubmit this with the comments address, most notably only enabling it for the uevents we want userspace to handle. Maybe including whatever code you have on the userspace side (even if it's an ugly prototype)

Quoted 58 lines · Christoph Hellwig

The uevent itself looks fine to me, but we really need to figure out how we can clear log pages in kernel space where needed.

We should already be doing this for the namespace changed one, and it will be very important at least for ANA as well.

A few more mechanical code comments below:

Quoted 1 line

+void nvme_uevent_work(struct nvme_ctrl *ctrl, int log)

static?

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

Quoted 12 lines

{

drivers/nvme/host/core.c +2 -1
 	u32 result = le32_to_cpu(res->u32);
+	u8 log_page = (result >> 16) & 8;
+	u8 event_type = result & 7;
 	bool done = true;
 
 	switch (le16_to_cpu(status) >> 1) {
@@ -2173,7 +2202,6 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		/*FALLTHRU*/
 	case NVME_SC_ABORT_REQ:
 		++ctrl->event_limit;
-		schedule_work(&ctrl->async_event_work);

This seems to disable any action on AERs in the kernel..

drivers/nvme/host/fc.c nvme_fc_complete_rq(struct request *rq) · line 1943

Quoted 13 lines
drivers/nvme/host/fc.c +2 -0
index 9690beb..3930a12 100644
@@ -1936,6 +1936,8 @@ nvme_fc_complete_rq(struct request *rq)
 			nvme_requeue_req(rq);
 			return;
 		}
+		if (req->errors & NVME_SC_MORE)
+			nvme_uevent(&dev->ctrl, NVME_LOG_ERROR);
 
 		if (blk_rq_is_passthrough(rq))
 			error = rq->errors;

We'll really need to factor the common request completion code into a helper in common code first..

include/linux/nvme.h line 19

Quoted 9 lines
include/linux/nvme.h +1 -0
index c43d435..e711de6 100644
@@ -16,6 +16,7 @@
 #define _LINUX_NVME_H
 
 #include <linux/types.h>
+#include <linux/kfifo.h>

This should move to drivers/nvme/host/nvme.h instead.

Quoted 4 lines

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

---end quoted text---

#3 lore raw

RFC PATCH 1/1 nvme: Submit uevents for log page notification

Quoted 4 lines · Christoph Hellwig

Given that we have the uevent discussion again can you resubmit this with the comments address, most notably only enabling it for the uevents we want userspace to handle. Maybe including whatever code you have on the userspace side (even if it's an ugly prototype)

Thanks for reminding me. I'll rebase, address the comments, and resend shortly.

maintainer
#4 lore raw

RFC PATCH 1/1 nvme: Submit uevents for log page notification

In reply to Keith Busch
Quoted 50 lines · Keith Busch

This is a first attempt at adding uevents to nvme. The concept was discussed at LSFMM, so here it is for consideration. :)

In this implementation, the driver will submit a "change" uevent whenever the controller indicates a log page contains pertinent information. This happens in response to an Asynchronouse Event Notification, or if a command completes with the "MORE" status bit set. If there are other events anyone thinks we'd like udev to get a chance to handle, or would prefer to see these variables submitted to udev in a different format, please let me know.

Submitting a uevent from the kernel can't be done from an irq context, which is the context the driver learns of such event, so this path enqueues the log identifier of internest on a FIFO, then has the async event work flush the event FIFO to udev. Pretty simple.

Tested with the following rule to kick an nvme-cli generic get-log to clear the log request and append the returned log data to a temporary log. This is just an example for testing and not intended for real life use.

ACTION=="change", SUBSYSTEM=="nvme", ENV{NVME_LOG}=="*", \
	RUN+="/bin/sh -c '/usr/local/sbin/nvme get-log $env{DEVNAME} --log-id=$env{NVME_LOG} --log-len=4096 >> /tmp/nvme-log'"
Signed-off-by Keith Busch <keith.busch at intel.com>
---
  drivers/nvme/host/core.c   | 35 +++++++++++++++++++++++++++++++++--
  drivers/nvme/host/fc.c     |  2 ++
  drivers/nvme/host/nvme.h   |  4 ++++
  drivers/nvme/host/pci.c    |  2 ++
  drivers/nvme/host/rdma.c   |  2 ++
  drivers/nvme/target/loop.c |  2 ++
  include/linux/nvme.h       |  2 ++
  7 files changed, 47 insertions(+), 2 deletions(-)
drivers/nvme/host/core.c +7 -0
index 9b3b57f..a757deb 100644
@@ -1879,6 +1879,27 @@ static const struct attribute_group *nvme_dev_attr_groups[] = {
  	NULL,
  };
 
+void nvme_uevent_work(struct nvme_ctrl *ctrl, int log)
+{
+	char buffer[13]; /* NVME_LOG=255\0 */
+	char *envp[2] = {buffer, NULL};
+
+	snprintf(buffer, sizeof(buffer), "NVME_LOG=%d", log);
+	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);

drivers/nvme/host/core.c static const struct attribute_group *nvme_dev_attr_groups[] = { · line 1898

Quoted 10 lines
drivers/nvme/host/core.c +10 -0
+}
+
+void nvme_uevent(struct nvme_ctrl *ctrl, int log_page)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+	kfifo_put(&ctrl->log_event_fifo, log_page);
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+

Check kfifo_put rc?

drivers/nvme/host/core.c static const struct attribute_group *nvme_dev_attr_groups[] = { · line 1899

Quoted 1 line
drivers/nvme/host/core.c +1 -0
+	schedule_work(&ctrl->async_event_work);

Can you explain why scheduling async_event_work (again) instead of having a separate work for it?

Also, we should use the nvme_wq here to avoid a warning on flush dependency violations on shutdown (see dab469a893f40cdd9d41f52515a4ffa745bef655).