[PATCHv2] nvme: always initialize known command effects

[PATCHv2] nvme: always initialize known command effects

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Instead of appending command effects flags per IO, set the known effects
flags the driver needs to react to just once during initial setup.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 83 +++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 39 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 7be562a4e1aa7..d7d2c2b342ba4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1060,41 +1060,12 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 }
 EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
 
-static u32 nvme_known_admin_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_admin_format_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
-			NVME_CMD_EFFECTS_CSE_MASK;
-	case nvme_admin_sanitize_nvm:
-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK;
-	default:
-		break;
-	}
-	return 0;
-}
-
-static u32 nvme_known_nvm_effects(u8 opcode)
-{
-	switch (opcode) {
-	case nvme_cmd_write:
-	case nvme_cmd_write_zeroes:
-	case nvme_cmd_write_uncor:
-		 return NVME_CMD_EFFECTS_LBCC;
-	default:
-		return 0;
-	}
-}
-
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 {
 	u32 effects = 0;
 
 	if (ns) {
-		if (ns->head->effects)
-			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
-		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
-			effects |= nvme_known_nvm_effects(opcode);
+		effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
 			dev_warn_once(ctrl->device,
 				"IO command:%02x has unusual effects:%08x\n",
@@ -1107,9 +1078,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
 		 */
 		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
 	} else {
-		if (ctrl->effects)
-			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
-		effects |= nvme_known_admin_effects(opcode);
+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
 	}
 
 	return effects;
@@ -3122,6 +3091,44 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
+{
+	struct nvme_effects_log	*log = ctrl->effects;
+
+	log->acs[nvme_admin_format_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_NCC |
+						NVME_CMD_EFFECTS_CSE_MASK);
+	log->acs[nvme_admin_sanitize_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
+						NVME_CMD_EFFECTS_CSE_MASK);
+
+	log->iocs[nvme_cmd_write] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	log->iocs[nvme_cmd_write_zeroes] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+	log->iocs[nvme_cmd_write_uncor] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
+}
+
+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
+{
+	int ret = 0;
+
+	if (ctrl->effects)
+		return 0;
+
+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!ctrl->effects) {
+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
+		if (!ctrl->effects)
+			return -ENOMEM;
+	}
+
+	nvme_init_known_nvm_effects(ctrl);
+	return 0;
+}
+
 static int nvme_init_identify(struct nvme_ctrl *ctrl)
 {
 	struct nvme_id_ctrl *id;
@@ -3135,12 +3142,6 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		return -EIO;
 	}
 
-	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
-		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
-		if (ret < 0)
-			goto out_free;
-	}
-
 	if (!(ctrl->ops->flags & NVME_F_FABRICS))
 		ctrl->cntlid = le16_to_cpu(id->cntlid);
 
@@ -3163,6 +3164,10 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ret = nvme_init_subsystem(ctrl, id);
 		if (ret)
 			goto out_free;
+
+		ret = nvme_init_effects(ctrl, id);
+		if (ret)
+			goto out_free;
 	}
 	memcpy(ctrl->subsys->firmware_rev, id->fr,
 	       sizeof(ctrl->subsys->firmware_rev));
-- 
2.30.2

Re: [PATCHv2] nvme: always initialize known command effects

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCHv2] nvme: always initialize known command effects

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
>From: Keith Busch <kbusch@kernel.org>
>
>Instead of appending command effects flags per IO, set the known effects
>flags the driver needs to react to just once during initial setup.
>
>Signed-off-by: Keith Busch <kbusch@kernel.org>
>---
> drivers/nvme/host/core.c | 83 +++++++++++++++++++++-------------------
> 1 file changed, 44 insertions(+), 39 deletions(-)
>
>diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>index 7be562a4e1aa7..d7d2c2b342ba4 100644
>--- a/drivers/nvme/host/core.c
>+++ b/drivers/nvme/host/core.c
>@@ -1060,41 +1060,12 @@ int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> }
> EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
>
>-static u32 nvme_known_admin_effects(u8 opcode)
>-{
>-	switch (opcode) {
>-	case nvme_admin_format_nvm:
>-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_NCC |
>-			NVME_CMD_EFFECTS_CSE_MASK;
>-	case nvme_admin_sanitize_nvm:
>-		return NVME_CMD_EFFECTS_LBCC | NVME_CMD_EFFECTS_CSE_MASK;
>-	default:
>-		break;
>-	}
>-	return 0;
>-}
>-
>-static u32 nvme_known_nvm_effects(u8 opcode)
>-{
>-	switch (opcode) {
>-	case nvme_cmd_write:
>-	case nvme_cmd_write_zeroes:
>-	case nvme_cmd_write_uncor:
>-		 return NVME_CMD_EFFECTS_LBCC;
>-	default:
>-		return 0;
>-	}
>-}
>-
> u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
> {
> 	u32 effects = 0;
>
> 	if (ns) {
>-		if (ns->head->effects)
>-			effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
>-		if (ns->head->ids.csi == NVME_CAP_CSS_NVM)
>-			effects |= nvme_known_nvm_effects(opcode);
>+		effects = le32_to_cpu(ns->head->effects->iocs[opcode]);
> 		if (effects & ~(NVME_CMD_EFFECTS_CSUPP | NVME_CMD_EFFECTS_LBCC))
> 			dev_warn_once(ctrl->device,
> 				"IO command:%02x has unusual effects:%08x\n",
>@@ -1107,9 +1078,7 @@ u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns, u8 opcode)
> 		 */
> 		effects &= ~NVME_CMD_EFFECTS_CSE_MASK;
> 	} else {
>-		if (ctrl->effects)
>-			effects = le32_to_cpu(ctrl->effects->acs[opcode]);
>-		effects |= nvme_known_admin_effects(opcode);
>+		effects = le32_to_cpu(ctrl->effects->acs[opcode]);
> 	}
>
> 	return effects;
>@@ -3122,6 +3091,44 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
> 	return ret;
> }
>
>+static void nvme_init_known_nvm_effects(struct nvme_ctrl *ctrl)
>+{
>+	struct nvme_effects_log	*log = ctrl->effects;
>+
>+	log->acs[nvme_admin_format_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
>+						NVME_CMD_EFFECTS_NCC |
>+						NVME_CMD_EFFECTS_CSE_MASK);
>+	log->acs[nvme_admin_sanitize_nvm] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC |
>+						NVME_CMD_EFFECTS_CSE_MASK);
>+
>+	log->iocs[nvme_cmd_write] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+	log->iocs[nvme_cmd_write_zeroes] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+	log->iocs[nvme_cmd_write_uncor] |= cpu_to_le32(NVME_CMD_EFFECTS_LBCC);
>+}
>+
>+static int nvme_init_effects(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>+{
>+	int ret = 0;
>+
>+	if (ctrl->effects)
>+		return 0;
>+
>+	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
>+		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
>+		if (ret < 0)
>+			return ret;
>+	}
>+
>+	if (!ctrl->effects) {
>+		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);

ctrl->effects is not getting freed if controller does not support the
commands-supported-and-effects log page?


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv2] nvme: always initialize known command effects

[Keith Busch]

On Mon, Jan 23, 2023 at 03:54:31PM +0530, Kanchan Joshi wrote:
> On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
> > +	if (!ctrl->effects) {
> > +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> 
> ctrl->effects is not getting freed if controller does not support the
> commands-supported-and-effects log page?

Right. If the controller doesn't support it, the driver will make one up
with some sane defaults. The point of this patch is that we don't want
to re-construct the defaults for each passthrough command so we need to
store the defaults somewhere.

Re: [PATCHv2] nvme: always initialize known command effects

[Christoph Hellwig]

On Mon, Jan 23, 2023 at 09:29:35AM -0700, Keith Busch wrote:
> On Mon, Jan 23, 2023 at 03:54:31PM +0530, Kanchan Joshi wrote:
> > On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
> > > +	if (!ctrl->effects) {
> > > +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> > 
> > ctrl->effects is not getting freed if controller does not support the
> > commands-supported-and-effects log page?
> 
> Right. If the controller doesn't support it, the driver will make one up
> with some sane defaults. The point of this patch is that we don't want
> to re-construct the defaults for each passthrough command so we need to
> store the defaults somewhere.

But it never ends up beeing freed even when the controller is torn
down as it never gets added to ctrl->cels.

Re: [PATCHv2] nvme: always initialize known command effects

[Christoph Hellwig]

On Mon, Jan 23, 2023 at 05:35:36PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2023 at 09:29:35AM -0700, Keith Busch wrote:
> > On Mon, Jan 23, 2023 at 03:54:31PM +0530, Kanchan Joshi wrote:
> > > On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
> > > > +	if (!ctrl->effects) {
> > > > +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> > > 
> > > ctrl->effects is not getting freed if controller does not support the
> > > commands-supported-and-effects log page?
> > 
> > Right. If the controller doesn't support it, the driver will make one up
> > with some sane defaults. The point of this patch is that we don't want
> > to re-construct the defaults for each passthrough command so we need to
> > store the defaults somewhere.
> 
> But it never ends up beeing freed even when the controller is torn
> down as it never gets added to ctrl->cels.

And maybe part of that is the weird double storage.  Can we do away
with ctrl->effects and head->effects and just do the xarray lookup?
The xarray lookup for our normal case will basically add one more
pointer chase and one more cache line that is read.

Re: [PATCHv2] nvme: always initialize known command effects

[Keith Busch]

On Mon, Jan 23, 2023 at 05:38:19PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2023 at 05:35:36PM +0100, Christoph Hellwig wrote:
> > On Mon, Jan 23, 2023 at 09:29:35AM -0700, Keith Busch wrote:
> > > On Mon, Jan 23, 2023 at 03:54:31PM +0530, Kanchan Joshi wrote:
> > > > On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
> > > > > +	if (!ctrl->effects) {
> > > > > +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> > > > 
> > > > ctrl->effects is not getting freed if controller does not support the
> > > > commands-supported-and-effects log page?
> > > 
> > > Right. If the controller doesn't support it, the driver will make one up
> > > with some sane defaults. The point of this patch is that we don't want
> > > to re-construct the defaults for each passthrough command so we need to
> > > store the defaults somewhere.
> > 
> > But it never ends up beeing freed even when the controller is torn
> > down as it never gets added to ctrl->cels.
> 
> And maybe part of that is the weird double storage.  Can we do away
> with ctrl->effects and head->effects and just do the xarray lookup?
> The xarray lookup for our normal case will basically add one more
> pointer chase and one more cache line that is read.

xarray lookups are pretty slow in comparison, though. This is in a fast
path now, so I think we need to keep caching it.

Alternatively, we could replace the xarray with a normal array sized to
highest seen CSI. Realisitically, I don't think we'll see a CSI higher
than 3 any time soon, so it wouldn't take up much space.

Re: [PATCHv2] nvme: always initialize known command effects

[Christoph Hellwig]

On Mon, Jan 23, 2023 at 10:42:45AM -0700, Keith Busch wrote:
> xarray lookups are pretty slow in comparison, though. This is in a fast
> path now, so I think we need to keep caching it.

Is it?

> Alternatively, we could replace the xarray with a normal array sized to
> highest seen CSI. Realisitically, I don't think we'll see a CSI higher
> than 3 any time soon, so it wouldn't take up much space.

The problem is that we lose the extensibility to arbitrary command
sets for the ng devices.  So if the xarray performance is a problem
let's not waste any time on this idea.

Re: [PATCHv2] nvme: always initialize known command effects

[Keith Busch]

On Mon, Jan 23, 2023 at 07:02:19PM +0100, Christoph Hellwig wrote:
> On Mon, Jan 23, 2023 at 10:42:45AM -0700, Keith Busch wrote:
> > xarray lookups are pretty slow in comparison, though. This is in a fast
> > path now, so I think we need to keep caching it.
> 
> Is it?

No, I was mistaken. I was thinking of uring_cmd for fast path, but that
doesn't go through here.

Re: [PATCHv2] nvme: always initialize known command effects

[Keith Busch]

On Mon, Jan 23, 2023 at 11:53:59AM -0700, Keith Busch wrote:
> On Mon, Jan 23, 2023 at 07:02:19PM +0100, Christoph Hellwig wrote:
> > On Mon, Jan 23, 2023 at 10:42:45AM -0700, Keith Busch wrote:
> > > xarray lookups are pretty slow in comparison, though. This is in a fast
> > > path now, so I think we need to keep caching it.
> > 
> > Is it?
> 
> No, I was mistaken. I was thinking of uring_cmd for fast path, but that
> doesn't go through here.

Oh, I'm an idiot: uring_cmd recently does now use this through
nvme_cmd_allowed(), so the cached effects are needed over the slower xarray
lookup.

Re: [PATCHv2] nvme: always initialize known command effects

[Keith Busch]

On Mon, Jan 23, 2023 at 03:54:31PM +0530, Kanchan Joshi wrote:
> On Thu, Jan 19, 2023 at 08:41:28AM -0800, Keith Busch wrote:
> > +	if (ctrl->effects)
> > +		return 0;
> > +
> > +	if (id->lpa & NVME_CTRL_LPA_CMD_EFFECTS_LOG) {
> > +		ret = nvme_get_effects_log(ctrl, NVME_CSI_NVM, &ctrl->effects);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	if (!ctrl->effects) {
> > +		ctrl->effects = kzalloc(sizeof(*ctrl->effects), GFP_KERNEL);
> 
> ctrl->effects is not getting freed if controller does not support the
> commands-supported-and-effects log page?

Or maybe you meant that it's never freed *ever* since it's not in the
"cels" structure? Oops, that is indeed a leak...