* [PATCHv2] nvme: always initialize known command effects @ 2023-01-19 16:41 ` Keith Busch 2023-01-23 9:21 ` Sagi Grimberg 2023-01-23 10:24 ` Kanchan Joshi 0 siblings, 2 replies; 11+ messages in thread From: Keith Busch @ 2023-01-19 16:41 UTC (permalink / raw) To: linux-nvme, hch, sagi; +Cc: 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-19 16:41 ` [PATCHv2] nvme: always initialize known command effects Keith Busch @ 2023-01-23 9:21 ` Sagi Grimberg 2023-01-23 10:24 ` Kanchan Joshi 1 sibling, 0 replies; 11+ messages in thread From: Sagi Grimberg @ 2023-01-23 9:21 UTC (permalink / raw) To: Keith Busch, linux-nvme, hch; +Cc: Keith Busch Reviewed-by: Sagi Grimberg <sagi@grimberg.me> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-19 16:41 ` [PATCHv2] nvme: always initialize known command effects Keith Busch 2023-01-23 9:21 ` Sagi Grimberg @ 2023-01-23 10:24 ` Kanchan Joshi 2023-01-23 16:29 ` Keith Busch 2023-01-23 16:33 ` Keith Busch 1 sibling, 2 replies; 11+ messages in thread From: Kanchan Joshi @ 2023-01-23 10:24 UTC (permalink / raw) To: Keith Busch; +Cc: linux-nvme, hch, sagi, Keith Busch [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 10:24 ` Kanchan Joshi @ 2023-01-23 16:29 ` Keith Busch 2023-01-23 16:35 ` Christoph Hellwig 2023-01-23 16:33 ` Keith Busch 1 sibling, 1 reply; 11+ messages in thread From: Keith Busch @ 2023-01-23 16:29 UTC (permalink / raw) To: Kanchan Joshi; +Cc: Keith Busch, linux-nvme, hch, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 16:29 ` Keith Busch @ 2023-01-23 16:35 ` Christoph Hellwig 2023-01-23 16:38 ` Christoph Hellwig 0 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2023-01-23 16:35 UTC (permalink / raw) To: Keith Busch; +Cc: Kanchan Joshi, Keith Busch, linux-nvme, hch, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 16:35 ` Christoph Hellwig @ 2023-01-23 16:38 ` Christoph Hellwig 2023-01-23 17:42 ` Keith Busch 0 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2023-01-23 16:38 UTC (permalink / raw) To: Keith Busch; +Cc: Kanchan Joshi, Keith Busch, linux-nvme, hch, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 16:38 ` Christoph Hellwig @ 2023-01-23 17:42 ` Keith Busch 2023-01-23 18:02 ` Christoph Hellwig 0 siblings, 1 reply; 11+ messages in thread From: Keith Busch @ 2023-01-23 17:42 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kanchan Joshi, Keith Busch, linux-nvme, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 17:42 ` Keith Busch @ 2023-01-23 18:02 ` Christoph Hellwig 2023-01-23 18:53 ` Keith Busch 0 siblings, 1 reply; 11+ messages in thread From: Christoph Hellwig @ 2023-01-23 18:02 UTC (permalink / raw) To: Keith Busch Cc: Christoph Hellwig, Kanchan Joshi, Keith Busch, linux-nvme, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 18:02 ` Christoph Hellwig @ 2023-01-23 18:53 ` Keith Busch 2023-01-24 21:42 ` Keith Busch 0 siblings, 1 reply; 11+ messages in thread From: Keith Busch @ 2023-01-23 18:53 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kanchan Joshi, Keith Busch, linux-nvme, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 18:53 ` Keith Busch @ 2023-01-24 21:42 ` Keith Busch 0 siblings, 0 replies; 11+ messages in thread From: Keith Busch @ 2023-01-24 21:42 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Kanchan Joshi, Keith Busch, linux-nvme, sagi 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] nvme: always initialize known command effects 2023-01-23 10:24 ` Kanchan Joshi 2023-01-23 16:29 ` Keith Busch @ 2023-01-23 16:33 ` Keith Busch 1 sibling, 0 replies; 11+ messages in thread From: Keith Busch @ 2023-01-23 16:33 UTC (permalink / raw) To: Kanchan Joshi; +Cc: Keith Busch, linux-nvme, hch, sagi 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... ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-01-24 21:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230119164739epcas5p17673bc9fb1b91d7c85561af220a230eb@epcas5p1.samsung.com>
2023-01-19 16:41 ` [PATCHv2] nvme: always initialize known command effects Keith Busch
2023-01-23 9:21 ` Sagi Grimberg
2023-01-23 10:24 ` Kanchan Joshi
2023-01-23 16:29 ` Keith Busch
2023-01-23 16:35 ` Christoph Hellwig
2023-01-23 16:38 ` Christoph Hellwig
2023-01-23 17:42 ` Keith Busch
2023-01-23 18:02 ` Christoph Hellwig
2023-01-23 18:53 ` Keith Busch
2023-01-24 21:42 ` Keith Busch
2023-01-23 16:33 ` Keith Busch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.