* [PATCH] nvme: cleanup nvme_configure_apst
@ 2021-04-09 9:45 Christoph Hellwig
2021-04-09 16:49 ` Niklas Cassel
2021-04-19 7:32 ` Niklas Cassel
0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2021-04-09 9:45 UTC (permalink / raw)
To: sagi, kbusch; +Cc: linux-nvme
Remove a level of indentation from the main code implementating the table
search by using a goto for the APST not supported case. Also move the
main comment above the function.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/nvme/host/core.c | 148 ++++++++++++++++++---------------------
1 file changed, 69 insertions(+), 79 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 314705da2c1076..78817190099040 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2550,28 +2550,28 @@ static int nvme_configure_acre(struct nvme_ctrl *ctrl)
return ret;
}
+/*
+ * APST (Autonomous Power State Transition) lets us program a table of power
+ * state transitions that the controller will perform automatically.
+ * We configure it with a simple heuristic: we are willing to spend at most 2%
+ * of the time transitioning between power states. Therefore, when running in
+ * any given state, we will enter the next lower-power non-operational state
+ * after waiting 50 * (enlat + exlat) microseconds, as long as that state's exit
+ * latency is under the requested maximum latency.
+ *
+ * We will not autonomously enter any non-operational state for which the total
+ * latency exceeds ps_max_latency_us.
+ *
+ * Users can set ps_max_latency_us to zero to turn off APST.
+ */
static int nvme_configure_apst(struct nvme_ctrl *ctrl)
{
- /*
- * APST (Autonomous Power State Transition) lets us program a
- * table of power state transitions that the controller will
- * perform automatically. We configure it with a simple
- * heuristic: we are willing to spend at most 2% of the time
- * transitioning between power states. Therefore, when running
- * in any given state, we will enter the next lower-power
- * non-operational state after waiting 50 * (enlat + exlat)
- * microseconds, as long as that state's exit latency is under
- * the requested maximum latency.
- *
- * We will not autonomously enter any non-operational state for
- * which the total latency exceeds ps_max_latency_us. Users
- * can set ps_max_latency_us to zero to turn off APST.
- */
-
- unsigned apste;
struct nvme_feat_auto_pst *table;
+ unsigned apste = 0;
u64 max_lat_us = 0;
+ __le64 target = 0;
int max_ps = -1;
+ int state;
int ret;
/*
@@ -2592,83 +2592,73 @@ static int nvme_configure_apst(struct nvme_ctrl *ctrl)
if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
/* Turn off APST. */
- apste = 0;
dev_dbg(ctrl->device, "APST disabled\n");
- } else {
- __le64 target = cpu_to_le64(0);
- int state;
-
- /*
- * Walk through all states from lowest- to highest-power.
- * According to the spec, lower-numbered states use more
- * power. NPSS, despite the name, is the index of the
- * lowest-power state, not the number of states.
- */
- for (state = (int)ctrl->npss; state >= 0; state--) {
- u64 total_latency_us, exit_latency_us, transition_ms;
-
- if (target)
- table->entries[state] = target;
-
- /*
- * Don't allow transitions to the deepest state
- * if it's quirked off.
- */
- if (state == ctrl->npss &&
- (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS))
- continue;
+ goto done;
+ }
- /*
- * Is this state a useful non-operational state for
- * higher-power states to autonomously transition to?
- */
- if (!(ctrl->psd[state].flags &
- NVME_PS_FLAGS_NON_OP_STATE))
- continue;
+ /*
+ * Walk through all states from lowest- to highest-power.
+ * According to the spec, lower-numbered states use more power. NPSS,
+ * despite the name, is the index of the lowest-power state, not the
+ * number of states.
+ */
+ for (state = (int)ctrl->npss; state >= 0; state--) {
+ u64 total_latency_us, exit_latency_us, transition_ms;
- exit_latency_us =
- (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
- if (exit_latency_us > ctrl->ps_max_latency_us)
- continue;
+ if (target)
+ table->entries[state] = target;
- total_latency_us =
- exit_latency_us +
- le32_to_cpu(ctrl->psd[state].entry_lat);
+ /*
+ * Don't allow transitions to the deepest state if it's quirked
+ * off.
+ */
+ if (state == ctrl->npss &&
+ (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS))
+ continue;
- /*
- * This state is good. Use it as the APST idle
- * target for higher power states.
- */
- transition_ms = total_latency_us + 19;
- do_div(transition_ms, 20);
- if (transition_ms > (1 << 24) - 1)
- transition_ms = (1 << 24) - 1;
+ /*
+ * Is this state a useful non-operational state for higher-power
+ * states to autonomously transition to?
+ */
+ if (!(ctrl->psd[state].flags & NVME_PS_FLAGS_NON_OP_STATE))
+ continue;
- target = cpu_to_le64((state << 3) |
- (transition_ms << 8));
+ exit_latency_us = (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
+ if (exit_latency_us > ctrl->ps_max_latency_us)
+ continue;
- if (max_ps == -1)
- max_ps = state;
+ total_latency_us = exit_latency_us +
+ le32_to_cpu(ctrl->psd[state].entry_lat);
- if (total_latency_us > max_lat_us)
- max_lat_us = total_latency_us;
- }
+ /*
+ * This state is good. Use it as the APST idle target for
+ * higher power states.
+ */
+ transition_ms = total_latency_us + 19;
+ do_div(transition_ms, 20);
+ if (transition_ms > (1 << 24) - 1)
+ transition_ms = (1 << 24) - 1;
+
+ target = cpu_to_le64((state << 3) | (transition_ms << 8));
+ if (max_ps == -1)
+ max_ps = state;
+ if (total_latency_us > max_lat_us)
+ max_lat_us = total_latency_us;
+ }
- apste = 1;
- if (max_ps == -1) {
- dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
- } else {
- dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
- max_ps, max_lat_us, (int)sizeof(*table), table);
- }
- }
+ if (max_ps == -1)
+ dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
+ else
+ dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
+ max_ps, max_lat_us, (int)sizeof(*table), table);
+ apste = 1;
+done:
ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
table, sizeof(*table), NULL);
if (ret)
dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
-
kfree(table);
return ret;
}
--
2.30.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: cleanup nvme_configure_apst
2021-04-09 9:45 [PATCH] nvme: cleanup nvme_configure_apst Christoph Hellwig
@ 2021-04-09 16:49 ` Niklas Cassel
2021-04-10 6:50 ` Christoph Hellwig
2021-04-19 7:07 ` Christoph Hellwig
2021-04-19 7:32 ` Niklas Cassel
1 sibling, 2 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-04-09 16:49 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, kbusch, linux-nvme
On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> Remove a level of indentation from the main code implementating the table
> search by using a goto for the APST not supported case. Also move the
> main comment above the function.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
(snip)
>
> +done:
> ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> table, sizeof(*table), NULL);
> if (ret)
> dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> -
> kfree(table);
> return ret;
> }
Since your patch includes whitespace cleanup related to apst,
perhaps you could remove the trailing whitespace after the
nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?
It's the only trailing whitespace that we have in all of core.c,
and it makes my eyes flinch everytime I see it ;)
Kind regards,
Niklas
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: cleanup nvme_configure_apst
2021-04-09 16:49 ` Niklas Cassel
@ 2021-04-10 6:50 ` Christoph Hellwig
2021-04-19 7:07 ` Christoph Hellwig
1 sibling, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2021-04-10 6:50 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Christoph Hellwig, sagi, kbusch, linux-nvme
On Fri, Apr 09, 2021 at 04:49:30PM +0000, Niklas Cassel wrote:
> > ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> > table, sizeof(*table), NULL);
> > if (ret)
> > dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> > -
> > kfree(table);
> > return ret;
> > }
>
> Since your patch includes whitespace cleanup related to apst,
> perhaps you could remove the trailing whitespace after the
> nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?
>
> It's the only trailing whitespace that we have in all of core.c,
> and it makes my eyes flinch everytime I see it ;)
Well, this patch only touches nvme_configure_apst, not any of the
callers. But I'd gladly take a patch to clean up the trailing
whitespace from you.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: cleanup nvme_configure_apst
2021-04-09 16:49 ` Niklas Cassel
2021-04-10 6:50 ` Christoph Hellwig
@ 2021-04-19 7:07 ` Christoph Hellwig
1 sibling, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2021-04-19 7:07 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Christoph Hellwig, sagi, kbusch, linux-nvme
Any chance to get a review on this patch?
On Fri, Apr 09, 2021 at 04:49:30PM +0000, Niklas Cassel wrote:
> On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> > Remove a level of indentation from the main code implementating the table
> > search by using a goto for the APST not supported case. Also move the
> > main comment above the function.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
>
> (snip)
>
> >
> > +done:
> > ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> > table, sizeof(*table), NULL);
> > if (ret)
> > dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> > -
> > kfree(table);
> > return ret;
> > }
>
> Since your patch includes whitespace cleanup related to apst,
> perhaps you could remove the trailing whitespace after the
> nvme_configure_apst() call (in nvme_init_ctrl_finish()) as well?
>
> It's the only trailing whitespace that we have in all of core.c,
> and it makes my eyes flinch everytime I see it ;)
>
>
> Kind regards,
> Niklas---end quoted text---
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] nvme: cleanup nvme_configure_apst
2021-04-09 9:45 [PATCH] nvme: cleanup nvme_configure_apst Christoph Hellwig
2021-04-09 16:49 ` Niklas Cassel
@ 2021-04-19 7:32 ` Niklas Cassel
1 sibling, 0 replies; 5+ messages in thread
From: Niklas Cassel @ 2021-04-19 7:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: sagi, kbusch, linux-nvme
On Fri, Apr 09, 2021 at 11:45:24AM +0200, Christoph Hellwig wrote:
> Remove a level of indentation from the main code implementating the table
> search by using a goto for the APST not supported case. Also move the
> main comment above the function.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/nvme/host/core.c | 148 ++++++++++++++++++---------------------
> 1 file changed, 69 insertions(+), 79 deletions(-)
>
(snip)
> + /*
> + * This state is good. Use it as the APST idle target for
> + * higher power states.
> + */
> + transition_ms = total_latency_us + 19;
> + do_div(transition_ms, 20);
> + if (transition_ms > (1 << 24) - 1)
> + transition_ms = (1 << 24) - 1;
> +
> + target = cpu_to_le64((state << 3) | (transition_ms << 8));
> + if (max_ps == -1)
> + max_ps = state;
> + if (total_latency_us > max_lat_us)
> + max_lat_us = total_latency_us;
> + }
>
First new line.
> - apste = 1;
>
Second new line.
> - if (max_ps == -1) {
> - dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
> - } else {
> - dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
> - max_ps, max_lat_us, (int)sizeof(*table), table);
> - }
> - }
> + if (max_ps == -1)
> + dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
> + else
> + dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
> + max_ps, max_lat_us, (int)sizeof(*table), table);
> + apste = 1;
>
> +done:
> ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
> table, sizeof(*table), NULL);
> if (ret)
> dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
> -
> kfree(table);
> return ret;
> }
If you remove the second new line, such that there are not two succeeding
new lines:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-19 7:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 9:45 [PATCH] nvme: cleanup nvme_configure_apst Christoph Hellwig
2021-04-09 16:49 ` Niklas Cassel
2021-04-10 6:50 ` Christoph Hellwig
2021-04-19 7:07 ` Christoph Hellwig
2021-04-19 7:32 ` Niklas Cassel
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.