nvme: allow to handle AER events by kernel drivers
RFC PATCH 1/1 nvme: allow to handle AER events by kernel drivers
In the current AER path, the kernel just picks up the event and re-sends the async event command to allow the device to report a new event. Sending the get log page to manage the event itself and clear the device flag is left to user space for what I can see.
In OCSSD we require some of these events to be handled by LightNVM targets, thus we need to pull the AER event up the stack in a meaningful way. The main challenge as I see it is moving the event in a clean way to the right driver.
This patch is a sketch on how to do this, very much work in progress. If we could add a bit to the result carried into nvme_result to signal which events are handled by the kernel, this would be much cleaner. From here on, drivers could register for specific events and then handled them accordingly.
Feedback at this early stage is very much appreciated.
Thanks!
Javier
Javier Gonz?lez (1):
nvme: allow lightnvm to have visibility over AER events
drivers/nvme/host/core.c | 37 +++++++++++++++++++++---- drivers/nvme/host/lightnvm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 3 ++ include/linux/lightnvm.h | 24 ++++++++++++++++ include/linux/nvme.h | 1 + 5 files changed, 125 insertions(+), 5 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
--- drivers/nvme/host/core.c | 37 +++++++++++++++++++++---- drivers/nvme/host/lightnvm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 3 ++ include/linux/lightnvm.h | 24 ++++++++++++++++ include/linux/nvme.h | 1 + 5 files changed, 125 insertions(+), 5 deletions(-)
drivers/nvme/host/core.c +32 -5
@@ -3263,15 +3263,31 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl)kfree (envp[0 ]); } -static void nvme_async_event_work (struct work_struct *work) +static void nvme_async_event (struct nvme_ctrl *ctrl) { -struct nvme_ctrl *ctrl = - container_of(work, struct nvme_ctrl, async_event_work); -nvme_aen_uevent (ctrl); ctrl->ops->submit_async_event (ctrl); } +static void nvme_async_event_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, async_event_work); + +nvme_async_event (ctrl); +} + +static void nvme_aer_handle_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, aer_handle_work); + +if (ctrl->aen_result &NVME_AER_NOTICE_LNVM_CHUNK ) +nvme_nvm_aer_handle (ctrl); + +nvme_async_event (ctrl); +} +static bool nvme_ctrl_pp_status (struct nvme_ctrl *ctrl) {
@@ -3362,7 +3378,16 @@ 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); + +/* TODO: Create a flag signaling the events that require kernel handling +* to make this treatment generic +*/ +if (result &NVME_AER_NOTICE_LNVM_CHUNK ) { + ctrl->aen_result = result; +queue_work (nvme_wq, &ctrl->aer_handle_work); + }else { +queue_work (nvme_wq, &ctrl->async_event_work); + } }EXPORT_SYMBOL_GPL (nvme_complete_async_event);
@@ -3370,6 +3395,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl) {nvme_stop_keep_alive (ctrl);flush_work (&ctrl->async_event_work); +flush_work (&ctrl->aer_handle_work);flush_work (&ctrl->scan_work);cancel_work_sync (&ctrl->fw_act_work);if (ctrl->ops->stop_ctrl)
@@ -3437,6 +3463,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_WORK (&ctrl->aer_handle_work, nvme_aer_handle_work);INIT_WORK (&ctrl->fw_act_work, nvme_fw_act_work);INIT_WORK (&ctrl->delete_work, nvme_delete_ctrl_work);
drivers/nvme/host/lightnvm.c +65 -0
77 lines not on this page · raw message
drivers/nvme/host/nvme.h +3 -0
@@ -183,6 +183,7 @@ struct nvme_ctrl {struct nvme_effects_log *effects;struct work_struct scan_work;struct work_struct async_event_work; +struct work_struct aer_handle_work;struct delayed_work ka_work;struct nvme_command ka_cmd;struct work_struct fw_act_work;
@@ -506,6 +507,7 @@ void nvme_nvm_unregister(struct nvme_ns *ns);int nvme_nvm_register_sysfs (struct nvme_ns *ns);void nvme_nvm_unregister_sysfs (struct nvme_ns *ns);int nvme_nvm_ioctl (struct nvme_ns *ns,unsigned int cmd,unsigned long arg); +void nvme_nvm_aer_handle (struct nvme_ctrl *ctrl);#else static inline void nvme_nvm_update_nvm_info (struct nvme_ns *ns) {}; static inline int nvme_nvm_register(struct nvme_ns *ns, char *disk_name,
@@ -525,6 +527,7 @@ static inline int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd, {return -ENOTTY ; } +static inline void nvme_nvm_aer_handle (struct nvme_ctrl *ctrl) {};#endif /* CONFIG_NVM */ static inline struct nvme_ns *nvme_get_ns_from_dev (struct device *dev)
include/linux/lightnvm.h +24 -0
@@ -233,6 +233,30 @@ struct nvm_addrf { };enum { +/* valid bit mask */ +NVM_LOGPAGE_STATE_MASK =0x3 , +NVM_LOGPAGE_SEVERITY_MASK =0x5 , + +/* scope */ +NVM_LOGPAGE_SCOPE_SECTOR =1 , +NVM_LOGPAGE_SCOPE_CHUNK =2 , +NVM_LOGPAGE_SCOPE_LUN =4 , + +/* severity */ +NVM_LOGPAGE_SEVERITY_LOW =1 , +NVM_LOGPAGE_SEVERITY_MID =2 , +NVM_LOGPAGE_SEVERITY_HIGH =4 , +NVM_LOGPAGE_SEVERITY_UNREC =8 , +NVM_LOGPAGE_SEVERITY_DEV =16 , +}; + +struct nvm_log_page { +struct ppa_addr ppa; +u16 scope; +u8 severity; +}; + +enum {/* Chunk states */ NVM_CHK_ST_FREE =1 <<0 ,NVM_CHK_ST_CLOSED =1 <<1 ,
include/linux/nvme.h +1 -0
@@ -440,6 +440,7 @@ enum {NVME_AER_VS =7 ,NVME_AER_NOTICE_NS_CHANGED =0x0002 ,NVME_AER_NOTICE_FW_ACT_STARTING =0x0102 , + NVME_AER_NOTICE_LNVM_CHUNK = 0xd00007, }; struct nvme_lba_range_type {
Signature · 1 line
2.7.4
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Quoted 31 lines · Javier Gonz?lez
--- drivers/nvme/host/core.c | 37 +++++++++++++++++++++---- drivers/nvme/host/lightnvm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 3 ++ include/linux/lightnvm.h | 24 ++++++++++++++++ include/linux/nvme.h | 1 + 5 files changed, 125 insertions(+), 5 deletions(-)drivers/nvme/host/core.c +6
-4@@ -3263,15 +3263,31 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl)kfree (envp[0 ]); } -static void nvme_async_event_work (struct work_struct *work) +static void nvme_async_event (struct nvme_ctrl *ctrl) { -struct nvme_ctrl *ctrl = - container_of(work, struct nvme_ctrl, async_event_work); -nvme_aen_uevent (ctrl); ctrl->ops->submit_async_event (ctrl); } +static void nvme_async_event_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, async_event_work); +
drivers/nvme/host/core.c static void nvme_aen_uevent(struct nvme_ctrl *ctrl) · line 3286
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+nvme_async_event (ctrl); +} + +static void nvme_aer_handle_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, aer_handle_work); + +if (ctrl->aen_result &NVME_AER_NOTICE_LNVM_CHUNK ) +nvme_nvm_aer_handle (ctrl);
Just throwing ideas out:
Does it make sense to have the lnvm core register an "aer callback" into the core driver? Then instead of calling the nvm_aer_handle directly we'd do like:
int res = 0;
if (child_driver->handle_aer)
res = child_driver->handle_aer();
if (!res)
/* normal nvme driver handle */
If we do it this way if Fabrics, or what ever comes
in the future could register aer handlers as well and we wouldn't have to constantly update this function.
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Quoted 53 lines · Scott Bauer
Quoted 31 lines · Javier Gonz?lez
---
drivers/nvme/host/core.c | 37 +++++++++++++++++++++---- drivers/nvme/host/lightnvm.c | 65 ++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 3 ++ include/linux/lightnvm.h | 24 ++++++++++++++++ include/linux/nvme.h | 1 + 5 files changed, 125 insertions(+), 5 deletions(-)drivers/nvme/host/core.c +6
-4@@ -3263,15 +3263,31 @@ static void nvme_aen_uevent(struct nvme_ctrl *ctrl)kfree (envp[0 ]); } -static void nvme_async_event_work (struct work_struct *work) +static void nvme_async_event (struct nvme_ctrl *ctrl) { -struct nvme_ctrl *ctrl = - container_of(work, struct nvme_ctrl, async_event_work); -nvme_aen_uevent (ctrl); ctrl->ops->submit_async_event (ctrl); } +static void nvme_async_event_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, async_event_work); +drivers/nvme/host/core.c static void nvme_aen_uevent(struct nvme_ctrl *ctrl) · line 3286
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+nvme_async_event (ctrl); +} + +static void nvme_aer_handle_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, aer_handle_work); + +if (ctrl->aen_result &NVME_AER_NOTICE_LNVM_CHUNK ) +nvme_nvm_aer_handle (ctrl);Just throwing ideas out:
Does it make sense to have the lnvm core register an "aer callback" into the core driver? Then instead of calling thenvm_aer_handledirectly we'd do like:int res = 0;
if (child_driver->handle_aer)
res =child_driver->handle_aer();
Quoted 7 lines
if (!res)
/* normal nvme driver handle */If we do it this way if Fabrics, or what ever comes
in the future could register aer handlers as well and we wouldn't have to constantly update this function.
If we can agree that having a similar logic to this workqueue is a good way of allowing the nvme driver to enable in-kernel AER handlers, then the structure you mention would be a good way to handle the same event by different registered drivers.
At this point though, there is no other in-kernel users (that I can see), who are interested in managing AER events. Maybe they start popping out soon.
Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20180413/25dbc185/attachment-0001.sig>
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
In reply to Javier GonzálezHi Javier,
We currently do have some logic in the core to say which events are sent to user space for handling and others that are handled internally. There's currently just two events with kernel specifc handling:
namespace inventory change, and firmware activation.
The example in your patch looks like one of the scenarios envisioned to be handled by a udev rule. Is that not going to work for you here?
If you do have an event that needs special handling, I think you'd want to filter that out of the user space notification and invoke the kernel callback specific to it your event.
drivers/nvme/host/core.c static void nvme_aen_uevent(struct nvme_ctrl *ctrl) · line 3286
Quoted 7 lines · Javier Gonz?lez
drivers/nvme/host/core.c +7
-0+static void nvme_aer_handle_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, aer_handle_work); + +if (ctrl->aen_result &NVME_AER_NOTICE_LNVM_CHUNK ) +nvme_nvm_aer_handle (ctrl);
This 'if' will evaluate to true for _any_ VS AEN result, so this doesn't look like the right criteria for an LNVM specific.
Quoted 8 lines
drivers/nvme/host/core.c +3
-0+nvme_async_event (ctrl); +} +static bool nvme_ctrl_pp_status (struct nvme_ctrl *ctrl) {@@ -3362,7 +3378,16 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,default :
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 3387
Quoted 10 lines
drivers/nvme/host/core.c +7
-1dev_warn (ctrl->device,"async event result %08x\n" , result); } -queue_work (nvme_wq, &ctrl->async_event_work); + +/* TODO: Create a flag signaling the events that require kernel handling +* to make this treatment generic +*/ +if (result &NVME_AER_NOTICE_LNVM_CHUNK ) { + ctrl->aen_result = result; +queue_work (nvme_wq, &ctrl->aer_handle_work);
If this event needs special kernel handling, you should filter it out in the switch statement above this code and directly invoke the special callback.
I'm a bit concerned about special handling on a vendor specific code, though. That could easily collide with another vendor's that has an entirely different meaning.
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Quoted 9 lines · Keith Busch
Hi Javier,
We currently do have some logic in the core to say which events are sent to user space for handling and others that are handled internally. There's currently just two events with kernel specifc handling:
namespace inventory change, and firmware activation.
Quoted 6 lines
The example in your patch looks like one of the scenarios envisioned to be handled by a udev rule. Is that not going to work for you here?
If you do have an event that needs special handling, I think you'd want to filter that out of the user space notification and invoke the kernel callback specific to it your event.
Can I plug a udev rule to an in-kernel driver? A concrete example of the events we will handle is pblk having to remap data from a chunk that is becoming unreliable.
Quoted 12 lines · Javier Gonz?lez
drivers/nvme/host/core.c static void nvme_aen_uevent(struct nvme_ctrl *ctrl) · line 3286
Quoted 7 lines
drivers/nvme/host/core.c +7
-0+static void nvme_aer_handle_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, aer_handle_work); + +if (ctrl->aen_result &NVME_AER_NOTICE_LNVM_CHUNK ) +nvme_nvm_aer_handle (ctrl);This 'if' will evaluate to true for _any_ VS AEN result, so this doesn't look like the right criteria for an LNVM specific.
See the next comment.
Quoted 7 lines
Quoted 7 lines
drivers/nvme/host/core.c +3
-0+nvme_async_event (ctrl); +} +static bool
nvme_ctrl_pp_status(struct nvme_ctrl *ctrl){
Quoted 16 lines
drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 3387
Quoted 12 lines
drivers/nvme/host/core.c +7
-1@@ -3362,7 +3378,16 @@ 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); + +/* TODO: Create a flag signaling the events that require kernel handling +* to make this treatment generic +*/ +if (result &NVME_AER_NOTICE_LNVM_CHUNK ) { + ctrl->aen_result = result; +queue_work (nvme_wq, &ctrl->aer_handle_work);If this event needs special kernel handling, you should filter it out in the switch statement above this code and directly invoke the special callback.
My thought with this is that a driver having to manage the event will need to send the get log page command, thus it will need a workque or similar; dealing with such logic in the driver makes it easier to maintain I think. Then at the nvme_aer_handle_work() level, we would filter out the events to the specific drivers.
Quoted 4 lines
I'm a bit concerned about special handling on a vendor specific code, though. That could easily collide with another vendor's that has an entirely different meaning.
Of course. None of the specific bits will be upstreamed until they become standarized. As I answer to Christoph, at this point I want to start building the foundation.
Thanks!
Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20180413/5a5013aa/attachment.sig>
Sagi Grimberg
maintainerPATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Javier,
Quoted 8 lines
Quoted 8 lines
We currently do have some logic in the core to say which events are sent to user space for handling and others that are handled internally. There's currently just two events with kernel specifc handling:
namespace inventory change, and firmware activation.The example in your patch looks like one of the scenarios envisioned to be handled by a udev rule. Is that not going to work for you here?
Quoted 7 lines
Quoted 3 lines
If you do have an event that needs special handling, I think you'd want to filter that out of the user space notification and invoke the kernel callback specific to it your event.
Can I plug a udev rule to an in-kernel driver? A concrete example of the events we will handle is pblk having to remap data from a chunk that is becoming unreliable.
You can plug user-space action and kernel interface for it. It looks pretty stateless..
Does pblk expose a char device? I think that user-space can issue the log page and then pass the information to the kernel..
Given that I do see also that nvmet would be a potential consumer for AER, maybe user-space would be a good place to handle all this...
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Quoted 15 lines · Sagi Grimberg
Javier,
Quoted 11 lines
Quoted 11 lines
We currently do have some logic in the core to say which events are sent to user space for handling and others that are handled internally. There's currently just two events with kernel specifc handling:
namespace inventory change, and firmware activation.The example in your patch looks like one of the scenarios envisioned to be handled by a udev rule. Is that not going to work for you here?
If you do have an event that needs special handling, I think you'd want to filter that out of the user space notification and invoke the kernel callback specific to it your event.
Quoted 7 lines
Quoted 3 lines
Can I plug a udev rule to an in-kernel driver? A concrete example of the events we will handle is pblk having to remap data from a chunk that is becoming unreliable.
You can plug user-space action and kernel interface for it. It looks pretty stateless..
Ok. I'll look into it. Thanks.
Quoted 3 lines
Does pblk expose a char device? I think that user-space can issue the log page and then pass the information to the kernel..
Not yet. But we can have a generic handle in user-space to send the get log page, then we can easily expose one.
Quoted 2 lines
Given that I do see also that nvmet would be a potential consumer for AER, maybe user-space would be a good place to handle all this...
Great.
Javier
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.infradead.org/pipermail/linux-nvme/attachments/20180416/1c052cdd/attachment.sig>
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
In reply to Javier GonzálezQuoted 34 lines
enum {
include/linux/lightnvm.h +24
-0+/* valid bit mask */ +NVM_LOGPAGE_STATE_MASK =0x3 , +NVM_LOGPAGE_SEVERITY_MASK =0x5 , + +/* scope */ +NVM_LOGPAGE_SCOPE_SECTOR =1 , +NVM_LOGPAGE_SCOPE_CHUNK =2 , +NVM_LOGPAGE_SCOPE_LUN =4 , + +/* severity */ +NVM_LOGPAGE_SEVERITY_LOW =1 , +NVM_LOGPAGE_SEVERITY_MID =2 , +NVM_LOGPAGE_SEVERITY_HIGH =4 , +NVM_LOGPAGE_SEVERITY_UNREC =8 , + NVM_LOGPAGE_SEVERITY_DEV = 16, +}; + +struct nvm_log_page { +struct ppa_addr ppa; +u16 scope; +u8 severity; +}; + +enum {/* Chunk states */ NVM_CHK_ST_FREE =1 <<0 , NVM_CHK_ST_CLOSED = 1 << 1,include/linux/nvme.h +0
-0
include/linux/nvme.h enum { · line 444
Quoted 5 lines
include/linux/nvme.h +1
-0@@ -440,6 +440,7 @@ enum {NVME_AER_VS =7 ,NVME_AER_NOTICE_NS_CHANGED =0x0002 ,NVME_AER_NOTICE_FW_ACT_STARTING =0x0102 , + NVME_AER_NOTICE_LNVM_CHUNK = 0xd00007, };
Non of these is in the NVMe spec or a ratified TP, so NAK.
PATCH 1/1 nvme: allow lightnvm to have visibility over AER events
Quoted 36 lines · Christoph Hellwig
Quoted 34 lines
enum {
include/linux/lightnvm.h +24
-0+/* valid bit mask */ +NVM_LOGPAGE_STATE_MASK =0x3 , +NVM_LOGPAGE_SEVERITY_MASK =0x5 , + +/* scope */ +NVM_LOGPAGE_SCOPE_SECTOR =1 , +NVM_LOGPAGE_SCOPE_CHUNK =2 , +NVM_LOGPAGE_SCOPE_LUN =4 , + +/* severity */ +NVM_LOGPAGE_SEVERITY_LOW =1 , +NVM_LOGPAGE_SEVERITY_MID =2 , +NVM_LOGPAGE_SEVERITY_HIGH =4 , +NVM_LOGPAGE_SEVERITY_UNREC =8 , + NVM_LOGPAGE_SEVERITY_DEV = 16, +}; + +struct nvm_log_page { +struct ppa_addr ppa; +u16 scope; +u8 severity; +}; + +enum {/* Chunk states */ NVM_CHK_ST_FREE =1 <<0 , NVM_CHK_ST_CLOSED = 1 << 1,include/linux/nvme.h +0
-0
Quoted 8 lines
include/linux/nvme.h enum { · line 444
Quoted 6 lines
include/linux/nvme.h +1
-0@@ -440,6 +440,7 @@ enum {NVME_AER_VS =7 ,NVME_AER_NOTICE_NS_CHANGED =0x0002 ,NVME_AER_NOTICE_FW_ACT_STARTING =0x0102 , + NVME_AER_NOTICE_LNVM_CHUNK = 0xd00007, };Non of these is in the NVMe spec or a ratified TP, so NAK.
I only included these to show an example. At this point I only intend to build the foundation, not upstream non-standard, as you point out. I should have made this more clear in the RFC message.