[PATCH 3/3] nvme: add KConfig options for debug features

[Chaitanya Kulkarni]

On 12/13/21 1:27 AM, Sagi Grimberg wrote:
> External email: Use caution opening links or attachments
> 
> 
>>>> Add KConfig menu option to enable and disable gencounter debug
>>>> feature that uses config NVME_DEBUG_USE_CID_GENCTR.
>>>>
>>>> Signed-off-by: Chaitanya Kulkarni <kch at nvidia.com>
>>>> ---
>>>>    drivers/nvme/host/Kconfig | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
>>>> index dc0450ca23a3..dfa2609b7006 100644
>>>> --- a/drivers/nvme/host/Kconfig
>>>> +++ b/drivers/nvme/host/Kconfig
>>>> @@ -1,4 +1,14 @@
>>>>    # SPDX-License-Identifier: GPL-2.0-only
>>>> +menu "Debug (Enable driver debug features)"
>>>> +config NVME_DEBUG_USE_CID_GENCTR
>>>> +     bool "Enable command ID gen counter for spurious request
>>>> completion"
>>>> +     depends on NVME_CORE
>>>> +     help
>>>> +       The NVM Express driver will use generation counter
>>>> +       when calculating the command id. This is needed to debug the
>>>> +       spurious request completions coming from a buggy controller.
>>>
>>> This is not just to debug - it is also to protect against such a
>>> controller. What is the purpose of this config option anyways?
>>> The main distributions will (as they should) enable it anyways...
>>
>> I can rewrite the text and rename it to "driver features".
> 
> It is not a feature.

that is what I thought, hence I kept it under debug.

> 
>> We are protecting against such a controller which is not stable
>> (buggy), i.e. it is doing things which it shouldn't be
>> doing at the first place.
> 
> Yes, this is exactly what we are doing.
> 
>> Consider a case if controller is not
>> buggy then it adds instructions in the fast path which are not
>> needed at all.
> 
> We keep repeating this, if the controller assumes _anything_ on
> the command identifier then it _is_ buggy, period.
> 

Completely agree on this.

What I mean buggy in this context is not related to assuming anything
with command identifier population, it meant controller that is
causing spurious completions -> buggy. The controller that is not
able to process non-seq command ids and assumes anything is out of
picture for this patch-series (for that nvme-cli skip approach is
fine), see below.

> Again, the fact that the controller is buggy is perfectly fine
> really, that's why we have quirks.
> 

A controller which is suffering from spurious completions needs
gencounter to debug the issue, using quirk and masking gencounter
should be avoided in this case.

> What happened to your nvme-cli argument? that would make it
> specific to that controller.
> 

That is completely different issue, the nvme-cli argument tries
to fix the issue with a controller that fails to process
non-sequntial command-id such as Apple ctrl and needs quirk.

This path-series has nothing to do with that issue.
This patch-series is only focused on keeping the fast path lean
and allowing user(s) to configure gencounter in the environment
where :-

1. Controller is stable.
2. Able to process the non-sequential command ids
    (hence no quirk needed) and does not assume anything about the
    command id population unlike Apple controller.
3. Does not suffer from spurious completion problem.

For this controller, user can disable gencounter and keep the fast
path lean.

> The host side cost is very cheap as shown before.
> 
>> A controller(s) that is used in the production environment goes
>> through qualification process from vendors and from the consumers
>> to make sure they are stable, something like spurious completions
>> detection is a basic part of the qualification,
> 
> I don't think that assuming that a production controller does not
> have any hidden bugs is a great practice, and controllers evolve during
> their production lifetime.
> 

That is why we should keep it configurable and let user decide.

>> hence we should
>> allow user to configure genctr than forcing additional
>> instructions in the fast path and keep this pattern for future
>> such cases.
> 
> I personally think that given that the worst-case option for such
> a bug is possible data-corruption then we should not make it optional,
> but I won't insist on it if others think otherwise.
> 

Even if we keep the gencouter on unconditionally once spurious
completion is posted, controller is already in the inconsistent
state, there is no guarantee that data will be consistent. In
fact what we need is some way of signaling uevent on spurious
completion so that userspace can get notification on the top
of your work with gencounter.

>> Maybe I didn't understand, can you please explain what are the
>> benefits of having gen-counter where controller is stable?
> 
> The benefit is that the host code does not "assume" anything
> about the controller.
> 
>> I'll wait to send V2 if you can suggest any other way
>> (than kconfig something like module param, I'm not sure) so user
>> can configure genctr calculations, please let me know.
> 
> As I said, the nvme-cli skip-genctr argument is perfectly valid in my
> mind.

That is for the broken controllers which are not able to process
non-seq command id generation like Apple, this patch is not for that.