[PATCH] nvme: implement support for nvme relaxed effects

[PATCH] nvme: implement support for nvme relaxed effects

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

NVM Express TP4167 provides a way for controllers to report a relaxed
execution constraint. Specifically, it notifies of exclusivity for IO
vs. admin commands instead of grouping these together. If set, then we
don't need to freeze IO in order to execute that admin command. The
freezing distrupts IO processes, so it's nice to avoid that if the
controller tells us we don't need to do that.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 4 ++++
 include/linux/nvme.h     | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 975245527c1fc..75927e8579857 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1153,6 +1153,10 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
 	} else {
 		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
+
+		/* Ignore execution restrictions if any relaxation bits are set */
+		if (effects & NVME_CMD_EFFECTS_CSER_MASK)
+			effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
 	}
 
 	return effects;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index bc605ec4a3fd0..3ef4053ea9500 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -646,6 +646,7 @@ enum {
 	NVME_CMD_EFFECTS_NCC		= 1 << 2,
 	NVME_CMD_EFFECTS_NIC		= 1 << 3,
 	NVME_CMD_EFFECTS_CCC		= 1 << 4,
+	NVME_CMD_EFFECTS_CSER_MASK	= GENMASK(15, 14),
 	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
 	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
 	NVME_CMD_EFFECTS_SCOPE_MASK	= GENMASK(31, 20),
-- 
2.34.1

Re: [PATCH] nvme: implement support for nvme relaxed effects

[Chaitanya Kulkarni]

Keith,

On 2/5/24 11:17, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> NVM Express TP4167 provides a way for controllers to report a relaxed
> execution constraint. Specifically, it notifies of exclusivity for IO
> vs. admin commands instead of grouping these together. If set, then we
> don't need to freeze IO in order to execute that admin command. The
> freezing distrupts IO processes, so it's nice to avoid that if the
> controller tells us we don't need to do that.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 4 ++++
>   include/linux/nvme.h     | 1 +
>   2 files changed, 5 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 975245527c1fc..75927e8579857 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1153,6 +1153,10 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
>   		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
>   	} else {
>   		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> +
> +		/* Ignore execution restrictions if any relaxation bits are set */

not a blocker, but I found following much cleaner and easy to search in 
spec :-

+               /*
+                *  Command Submission and Execution Relaxation (CSER) takes
+                *  priority over Command Submission and Execution (CSE).
+                */

but if others are okay with original comment, looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

> +		if (effects & NVME_CMD_EFFECTS_CSER_MASK)
> +			effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
>   	}
>   
>   	return effects;
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index bc605ec4a3fd0..3ef4053ea9500 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -646,6 +646,7 @@ enum {
>   	NVME_CMD_EFFECTS_NCC		= 1 << 2,
>   	NVME_CMD_EFFECTS_NIC		= 1 << 3,
>   	NVME_CMD_EFFECTS_CCC		= 1 << 4,
> +	NVME_CMD_EFFECTS_CSER_MASK	= GENMASK(15, 14),
>   	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
>   	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
>   	NVME_CMD_EFFECTS_SCOPE_MASK	= GENMASK(31, 20),

Re: [PATCH] nvme: implement support for nvme relaxed effects

[Chaitanya Kulkarni]

>> +		if (effects & NVME_CMD_EFFECTS_CSER_MASK)
>> +			effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
>>    	}

looking at the code again we are not using NVME_CMD_EFFECTS_CSER_MASK
anywhere in the code to guarantee the admin-cmd exclusive execution
guarantee, is there a specific reason for that ?

According to spec:-
Figure 221: Command Supported and Effects data structure

*Bits 15:14
*Value :- 01b
*Definition :-
The command associated with this structure should only be submitted
when there is no outstanding Admin command that affects any namespace
and no Admin command should be submitted that affects any namespace
until this command is complete.

IOW, don't we need to freeze the admin queue when controller sets the CSER
value to 01b until command is completed ? or it is not done/needed for
a specific reason ?
>>    	return effects;
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index bc605ec4a3fd0..3ef4053ea9500 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -646,6 +646,7 @@ enum {
>>    	NVME_CMD_EFFECTS_NCC		= 1 << 2,
>>    	NVME_CMD_EFFECTS_NIC		= 1 << 3,
>>    	NVME_CMD_EFFECTS_CCC		= 1 << 4,
>> +	NVME_CMD_EFFECTS_CSER_MASK	= GENMASK(15, 14),
>>    	NVME_CMD_EFFECTS_CSE_MASK	= GENMASK(18, 16),
>>    	NVME_CMD_EFFECTS_UUID_SEL	= 1 << 19,
>>    	NVME_CMD_EFFECTS_SCOPE_MASK	= GENMASK(31, 20),
>

-ck

Re: [PATCH] nvme: implement support for nvme relaxed effects

[Keith Busch]

On Tue, Feb 06, 2024 at 08:34:39AM +0000, Chaitanya Kulkarni wrote:
> >> +		if (effects & NVME_CMD_EFFECTS_CSER_MASK)
> >> +			effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
> >>    	}
> 
> looking at the code again we are not using NVME_CMD_EFFECTS_CSER_MASK
> anywhere in the code to guarantee the admin-cmd exclusive execution
> guarantee, is there a specific reason for that ?
> 
> According to spec:-
> Figure 221: Command Supported and Effects data structure
> 
> *Bits 15:14
> *Value :- 01b
> *Definition :-
> The command associated with this structure should only be submitted
> when there is no outstanding Admin command that affects any namespace
> and no Admin command should be submitted that affects any namespace
> until this command is complete.
> 
> IOW, don't we need to freeze the admin queue when controller sets the CSER
> value to 01b until command is completed ? or it is not done/needed for
> a specific reason ?

The user is trying to submit an admin command, which wouldn't be
possible if we freeze the admin queue. We'd need another mechanism to
allow just the user's command, then a way to defeat it if that command
times out (we send admin commands as part of a reset sequence).

All that adds complications to the driver, and since there hasn't been
any communicated need to have the driver enforce this policy, we just
leave it up to user space to know what they're doing.

Re: [PATCH] nvme: implement support for nvme relaxed effects

[Chaitanya Kulkarni]

>> The command associated with this structure should only be submitted
>> when there is no outstanding Admin command that affects any namespace
>> and no Admin command should be submitted that affects any namespace
>> until this command is complete.
>>
>> IOW, don't we need to freeze the admin queue when controller sets the CSER
>> value to 01b until command is completed ? or it is not done/needed for
>> a specific reason ?
> 
> The user is trying to submit an admin command, which wouldn't be
> possible if we freeze the admin queue. We'd need another mechanism to
> allow just the user's command, then a way to defeat it if that command
> times out (we send admin commands as part of a reset sequence).
> 
> All that adds complications to the driver, and since there hasn't been
> any communicated need to have the driver enforce this policy, we just
> leave it up to user space to know what they're doing.

I did write a patch to see if we can do it without adding bunch of code,
but no, it doesn't really bring that much of a benefit vs complicating
exiting code as mentioned above.

I think this patch is okay then.

-ck

Re: [PATCH] nvme: implement support for nvme relaxed effects

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>