[PATCH] nvme-pci: trigger disk activity LED

[PATCH] nvme-pci: trigger disk activity LED

[Enzo Matsumiya]

Users can enable an LED to indicate I/O activity on NVMe PCIe devices:

  # echo "disk-activity" > /sys/class/leds/<led>/trigger

for the composite activity, or disk-{read,write} for individual
activities/LEDs.

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>
---
 drivers/nvme/host/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6a99ed680915..3e49d5980beb 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -15,6 +15,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/leds.h>
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -1037,6 +1038,8 @@ static __always_inline void nvme_pci_unmap_rq(struct request *req)
 			       rq_integrity_vec(req)->bv_len, rq_data_dir(req));
 	if (blk_rq_nr_phys_segments(req))
 		nvme_unmap_data(dev, req);
+
+	ledtrig_disk_activity(req_op(req) == REQ_OP_WRITE);
 }
 
 static void nvme_pci_complete_rq(struct request *req)
-- 
2.34.1

Re: [PATCH] nvme-pci: trigger disk activity LED

[Christoph Hellwig]

I don't think we should add code to the absolutel fast path for
blinkenlights.

Re: [PATCH] nvme-pci: trigger disk activity LED

[Jens Axboe]

On 2/28/22 2:22 AM, Christoph Hellwig wrote:
> I don't think we should add code to the absolutel fast path for
> blinkenlights.

Agree. It'd be a lot better to put the cost on the led trigger
side, and not need anything in the fast path for block devices.
Monitor disk stats, or something like that.

-- 
Jens Axboe

Re: [PATCH] nvme-pci: trigger disk activity LED

[Enzo Matsumiya]

On 02/28, Jens Axboe wrote:
>On 2/28/22 2:22 AM, Christoph Hellwig wrote:
>> I don't think we should add code to the absolutel fast path for
>> blinkenlights.
>
>Agree. It'd be a lot better to put the cost on the led trigger
>side, and not need anything in the fast path for block devices.
>Monitor disk stats, or something like that.

There's been at least 4 attempts to do so, as far as I'm aware (one of
them being mine). All got rejected due to the complexity it introduced,
that's how I ended up with this one-liner.

Performance-wise, I'm understand the problems, but according to ftrace,
ledtrig_disk_activity() adds an average of 0.2us overhead, whether an
LED is assigned or not. Is that really unacceptable?

If so, would introducing a CONFIG_NVME_LED (default =n) and wrap that
call around it make it better? Then at least there's a chance to inform
users that desires this feature about performance costs.


Cheers,

Enzo

Re: [PATCH] nvme-pci: trigger disk activity LED

[Jens Axboe]

On 2/28/22 8:30 PM, Enzo Matsumiya wrote:
> On 02/28, Jens Axboe wrote:
>> On 2/28/22 2:22 AM, Christoph Hellwig wrote:
>>> I don't think we should add code to the absolutel fast path for
>>> blinkenlights.
>>
>> Agree. It'd be a lot better to put the cost on the led trigger
>> side, and not need anything in the fast path for block devices.
>> Monitor disk stats, or something like that.
> 
> There's been at least 4 attempts to do so, as far as I'm aware (one of
> them being mine). All got rejected due to the complexity it introduced,
> that's how I ended up with this one-liner.
> 
> Performance-wise, I'm understand the problems, but according to ftrace,
> ledtrig_disk_activity() adds an average of 0.2us overhead, whether an
> LED is assigned or not. Is that really unacceptable?

On fast devices, we can complete a full IO in ~3us. If it's 6-7% of
overhead for that case, then yes, that is definitely unacceptable.

It's as much the principle of it. If it can be done in such a way that a
feature that almost nobody would use doesn't add to the fast path, then
it should be done that way.

What kind of frequency does this need to be toggled at? Surely not
hundreds of thousands or even million times per second? If it's suitably
low, then a registration scheme and a running timer would be a much
better idea. Each time the timer triggers, check devices you are
interested in and toggle the LED based on that. No fast path additions
for that, and it keeps the cost at zero for folks that don't need an LED
trigger for drive activity.

> If so, would introducing a CONFIG_NVME_LED (default =n) and wrap that
> call around it make it better? Then at least there's a chance to inform
> users that desires this feature about performance costs.

Doesn't really help, because then distros turn it on and we're back
where we would be if we didn't have a config option...

-- 
Jens Axboe