Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4100a868-c5bd-91dd-0c45-a92fb1344b12@kernel.dk>
Date:   Mon, 28 Feb 2022 20:36:40 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH] nvme-pci: trigger disk activity LED
Content-Language: en-US
To:     Enzo Matsumiya <ematsumiya@suse.de>
Cc:     Christoph Hellwig <hch@lst.de>, linux-nvme@lists.infradead.org,
        Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@fb.com>,
        Sagi Grimberg <sagi@grimberg.me>, linux-kernel@vger.kernel.org
References: <20220227234258.24619-1-ematsumiya@suse.de>
 <20220228092215.GA8549@lst.de>
 <36cfd242-6bb0-0af6-0faf-946c79baa378@kernel.dk>
 <20220301033001.tozk6cakdznww6wi@cyberdelia>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <20220301033001.tozk6cakdznww6wi@cyberdelia>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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