diff for duplicates of <1472483266.2816.14.camel@linux.intel.com> diff --git a/a/1.txt b/N1/1.txt index 91e4bbc..75f3b11 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,12 +1,12 @@ -On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: -> NVME devices can advertise multiple power states.??These states can +On Mon, 2016-08-29 at 02:25 -0700, Andy Lutomirski wrote: +> NVME devices can advertise multiple power states. These states can > be either "operational" (the device is fully functional but possibly > slow) or "non-operational" (the device is asleep until woken up). > Some devices can automatically enter a non-operational state when > idle for a specified amount of time and then automatically wake back > up when needed. > -> The hardware configuration is a table.??For each state, an entry in +> The hardware configuration is a table. For each state, an entry in > the table indicates the next deeper non-operational state, if any, > to autonomously transition to and the idle time required before > transitioning. @@ -14,9 +14,9 @@ On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: > This patch teaches the driver to program APST so that each > successive non-operational state will be entered after an idle time > equal to 100% of the total latency (entry plus exit) associated with -> that state.??A sysfs attribute 'apst_max_latency_ns' gives the +> that state. A sysfs attribute 'apst_max_latency_ns' gives the > maximum acceptable latency in ns; non-operational states with total -> latency greater than this value will not be used.??As a special +> latency greater than this value will not be used. As a special > case, apst_max_latency_ns=0 will disable APST entirely. > > On hardware without APST support, apst_max_latency_ns will not be @@ -24,9 +24,9 @@ On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: > > In theory, the device can expose "default" APST table, but this > doesn't seem to function correctly on my device (Samsung 950), nor -> does it seem particularly useful.??There is also an optional +> does it seem particularly useful. There is also an optional > mechanism by which a configuration can be "saved" so it will be -> automatically loaded on reset.??This can be configured from +> automatically loaded on reset. This can be configured from > userspace, but it doesn't seem useful to support in the driver. > > On my laptop, enabling APST seems to save nearly 1W. @@ -36,13 +36,13 @@ On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: > 'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST > configuration. > -> Signed-off-by: Andy Lutomirski <luto at kernel.org> +> Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- -> ?drivers/nvme/host/core.c | 167 +> drivers/nvme/host/core.c | 167 > +++++++++++++++++++++++++++++++++++++++++++++++ -> ?drivers/nvme/host/nvme.h |???6 ++ -> ?include/linux/nvme.h?????|???6 ++ -> ?3 files changed, 179 insertions(+) +> drivers/nvme/host/nvme.h | 6 ++ +> include/linux/nvme.h | 6 ++ +> 3 files changed, 179 insertions(+) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 3f7561ab54dc..042137ad2437 100644 @@ -50,31 +50,31 @@ On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: > +++ b/drivers/nvme/host/core.c > @@ -1223,6 +1223,98 @@ static void nvme_set_queue_limits(struct > nvme_ctrl *ctrl, -> ? blk_queue_write_cache(q, vwc, vwc); -> ?} -> ? +> blk_queue_write_cache(q, vwc, vwc); +> } +> > +static void nvme_configure_apst(struct nvme_ctrl *ctrl) > +{ > + /* -> + ?* APST (Autonomous Power State Transition) lets us program +> + * 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 +> + * 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 100 * (enlat + exlat) -> + ?* microseconds, as long as that state's total latency is +> + * in any given state, we will enter the next lower-power +> + * non-operational state after waiting 100 * (enlat + exlat) +> + * microseconds, as long as that state's total latency is > under -> + ?* the requested maximum latency. -> + ?* -> + ?* We will not autonomously enter any non-operational state +> + * the requested maximum latency. +> + * +> + * We will not autonomously enter any non-operational state > for -> + ?* which the total latency exceeds -> apst_max_latency_ns.??Users -> + ?* can set apst_max_latency_ns to zero to turn off APST. -> + ?*/ +> + * which the total latency exceeds +> apst_max_latency_ns. Users +> + * can set apst_max_latency_ns to zero to turn off APST. +> + */ > + > + unsigned apste; > + struct nvme_feat_auto_pst *table; @@ -87,16 +87,16 @@ On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote: > + dev_warn(ctrl->device, "NPSS is invalid; disabling > APST\n"); -Quick question. ?A little bit below in a later if() block, apste is set +Quick question. A little bit below in a later if() block, apste is set to 0 to turn off APST, which is to be used later in a -nvme_set_features() call to actually turn it off. ?You wouldn't want to +nvme_set_features() call to actually turn it off. You wouldn't want to also set apste to zero too and call a nvme_set_features() to "disable APST"? I guess I'm a little confused on the error statement, "disabling APST", when it doesn't seem like anything is being done to actually disable APST, it's just more of an invalid state retrieved from the HW. -? + > + return; > + } @@ -113,14 +113,14 @@ APST, it's just more of an invalid state retrieved from the HW. > + int state; > + > + /* -> + ?* Walk through all states from lowest- to highest- +> + * Walk through all states from lowest- to highest- > power. -> + ?* According to the spec, lower-numbered states use +> + * According to the spec, lower-numbered states use > more -> + ?* power.??NPSS, despite the name, is the index of +> + * power. NPSS, despite the name, is the index of > the -> + ?* lowest-power state, not the number of states. -> + ?*/ +> + * lowest-power state, not the number of states. +> + */ > + for (state = (int)ctrl->npss; state >= 0; state--) { > + u64 total_latency_us, transition_ms; > + @@ -128,13 +128,13 @@ APST, it's just more of an invalid state retrieved from the HW. > + table->entries[state] = target; > + > + /* -> + ?* Is this state a useful non-operational +> + * Is this state a useful non-operational > state for -> + ?* higher-power states to autonomously +> + * higher-power states to autonomously > transition to? -> + ?*/ +> + */ > + if (!(ctrl->psd[state].flags & 2)) -> + continue;??/* It's an operational +> + continue; /* It's an operational > state. */ > + > + total_latency_us = @@ -147,10 +147,10 @@ APST, it's just more of an invalid state retrieved from the HW. > + continue; > + > + /* -> + ?* This state is good.??Use it as the APST +> + * This state is good. Use it as the APST > idle -> + ?* target for higher power states. -> + ?*/ +> + * target for higher power states. +> + */ > + transition_ms = total_latency_us + 19; > + do_div(transition_ms, 20); > + if (transition_ms >= (1 << 24)) @@ -161,7 +161,7 @@ once? > + > + target = cpu_to_le64((state << 3) | -> + ?????(transition_ms << 8)); +> + (transition_ms << 8)); > + } > @@ -171,12 +171,12 @@ snip... . > + /* -> + ?* By default, allow up to 25ms of APST-induced -> latency.??This will -> + ?* have no effect on non-APST supporting controllers (i.e. +> + * By default, allow up to 25ms of APST-induced +> latency. This will +> + * have no effect on non-APST supporting controllers (i.e. > any -> + ?* controller with APSTA == 0). -> + ?*/ +> + * controller with APSTA == 0). +> + */ > + ctrl->apst_max_latency_ns = 25000000; Is it possible to make that a #define please? @@ -184,4 +184,4 @@ Is it possible to make that a #define please? Nice stuff! ->? +> diff --git a/a/content_digest b/N1/content_digest index e555a1b..eaa4aa4 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,19 +1,25 @@ "ref\0cover.1472462539.git.luto@kernel.org\0" "ref\088cc7972617eec58a81877d933604e0f0e342e43.1472462539.git.luto@kernel.org\0" - "From\0james_p_freyensee@linux.intel.com (J Freyensee)\0" - "Subject\0[PATCH 3/3] nvme: Enable autonomous power state transitions\0" + "From\0J Freyensee <james_p_freyensee@linux.intel.com>\0" + "Subject\0Re: [PATCH 3/3] nvme: Enable autonomous power state transitions\0" "Date\0Mon, 29 Aug 2016 08:07:46 -0700\0" + "To\0Andy Lutomirski <luto@kernel.org>" + Keith Busch <keith.busch@intel.com> + " Jens Axboe <axboe@fb.com>\0" + "Cc\0Christoph Hellwig <hch@lst.de>" + linux-nvme@lists.infradead.org + " linux-kernel@vger.kernel.org\0" "\00:1\0" "b\0" - "On Mon, 2016-08-29@02:25 -0700, Andy Lutomirski wrote:\n" - "> NVME devices can advertise multiple power states.??These states can\n" + "On Mon, 2016-08-29 at 02:25 -0700, Andy Lutomirski wrote:\n" + "> NVME devices can advertise multiple power states.\302\240\302\240These states can\n" "> be either \"operational\" (the device is fully functional but possibly\n" "> slow) or \"non-operational\" (the device is asleep until woken up).\n" "> Some devices can automatically enter a non-operational state when\n" "> idle for a specified amount of time and then automatically wake back\n" "> up when needed.\n" "> \n" - "> The hardware configuration is a table.??For each state, an entry in\n" + "> The hardware configuration is a table.\302\240\302\240For each state, an entry in\n" "> the table indicates the next deeper non-operational state, if any,\n" "> to autonomously transition to and the idle time required before\n" "> transitioning.\n" @@ -21,9 +27,9 @@ "> This patch teaches the driver to program APST so that each\n" "> successive non-operational state will be entered after an idle time\n" "> equal to 100% of the total latency (entry plus exit) associated with\n" - "> that state.??A sysfs attribute 'apst_max_latency_ns' gives the\n" + "> that state.\302\240\302\240A sysfs attribute 'apst_max_latency_ns' gives the\n" "> maximum acceptable latency in ns; non-operational states with total\n" - "> latency greater than this value will not be used.??As a special\n" + "> latency greater than this value will not be used.\302\240\302\240As a special\n" "> case, apst_max_latency_ns=0 will disable APST entirely.\n" "> \n" "> On hardware without APST support, apst_max_latency_ns will not be\n" @@ -31,9 +37,9 @@ "> \n" "> In theory, the device can expose \"default\" APST table, but this\n" "> doesn't seem to function correctly on my device (Samsung 950), nor\n" - "> does it seem particularly useful.??There is also an optional\n" + "> does it seem particularly useful.\302\240\302\240There is also an optional\n" "> mechanism by which a configuration can be \"saved\" so it will be\n" - "> automatically loaded on reset.??This can be configured from\n" + "> automatically loaded on reset.\302\240\302\240This can be configured from\n" "> userspace, but it doesn't seem useful to support in the driver.\n" "> \n" "> On my laptop, enabling APST seems to save nearly 1W.\n" @@ -43,13 +49,13 @@ "> 'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST\n" "> configuration.\n" "> \n" - "> Signed-off-by: Andy Lutomirski <luto at kernel.org>\n" + "> Signed-off-by: Andy Lutomirski <luto@kernel.org>\n" "> ---\n" - "> ?drivers/nvme/host/core.c | 167\n" + "> \302\240drivers/nvme/host/core.c | 167\n" "> +++++++++++++++++++++++++++++++++++++++++++++++\n" - "> ?drivers/nvme/host/nvme.h |???6 ++\n" - "> ?include/linux/nvme.h?????|???6 ++\n" - "> ?3 files changed, 179 insertions(+)\n" + "> \302\240drivers/nvme/host/nvme.h |\302\240\302\240\302\2406 ++\n" + "> \302\240include/linux/nvme.h\302\240\302\240\302\240\302\240\302\240|\302\240\302\240\302\2406 ++\n" + "> \302\2403 files changed, 179 insertions(+)\n" "> \n" "> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c\n" "> index 3f7561ab54dc..042137ad2437 100644\n" @@ -57,31 +63,31 @@ "> +++ b/drivers/nvme/host/core.c\n" "> @@ -1223,6 +1223,98 @@ static void nvme_set_queue_limits(struct\n" "> nvme_ctrl *ctrl,\n" - "> ?\tblk_queue_write_cache(q, vwc, vwc);\n" - "> ?}\n" - "> ?\n" + "> \302\240\tblk_queue_write_cache(q, vwc, vwc);\n" + "> \302\240}\n" + "> \302\240\n" "> +static void nvme_configure_apst(struct nvme_ctrl *ctrl)\n" "> +{\n" "> +\t/*\n" - "> +\t?* APST (Autonomous Power State Transition) lets us program\n" + "> +\t\302\240* APST (Autonomous Power State Transition) lets us program\n" "> a\n" - "> +\t?* table of power state transitions that the controller will\n" - "> +\t?* perform automatically.??We configure it with a simple\n" - "> +\t?* heuristic: we are willing to spend at most 2% of the time\n" - "> +\t?* transitioning between power states.??Therefore, when\n" + "> +\t\302\240* table of power state transitions that the controller will\n" + "> +\t\302\240* perform automatically.\302\240\302\240We configure it with a simple\n" + "> +\t\302\240* heuristic: we are willing to spend at most 2% of the time\n" + "> +\t\302\240* transitioning between power states.\302\240\302\240Therefore, when\n" "> running\n" - "> +\t?* in any given state, we will enter the next lower-power\n" - "> +\t?* non-operational state after waiting 100 * (enlat + exlat)\n" - "> +\t?* microseconds, as long as that state's total latency is\n" + "> +\t\302\240* in any given state, we will enter the next lower-power\n" + "> +\t\302\240* non-operational state after waiting 100 * (enlat + exlat)\n" + "> +\t\302\240* microseconds, as long as that state's total latency is\n" "> under\n" - "> +\t?* the requested maximum latency.\n" - "> +\t?*\n" - "> +\t?* We will not autonomously enter any non-operational state\n" + "> +\t\302\240* the requested maximum latency.\n" + "> +\t\302\240*\n" + "> +\t\302\240* We will not autonomously enter any non-operational state\n" "> for\n" - "> +\t?* which the total latency exceeds\n" - "> apst_max_latency_ns.??Users\n" - "> +\t?* can set apst_max_latency_ns to zero to turn off APST.\n" - "> +\t?*/\n" + "> +\t\302\240* which the total latency exceeds\n" + "> apst_max_latency_ns.\302\240\302\240Users\n" + "> +\t\302\240* can set apst_max_latency_ns to zero to turn off APST.\n" + "> +\t\302\240*/\n" "> +\n" "> +\tunsigned apste;\n" "> +\tstruct nvme_feat_auto_pst *table;\n" @@ -94,16 +100,16 @@ "> +\t\tdev_warn(ctrl->device, \"NPSS is invalid; disabling\n" "> APST\\n\");\n" "\n" - "Quick question. ?A little bit below in a later if() block, apste is set\n" + "Quick question. \302\240A little bit below in a later if() block, apste is set\n" "to 0 to turn off APST, which is to be used later in a\n" - "nvme_set_features() call to actually turn it off. ?You wouldn't want to\n" + "nvme_set_features() call to actually turn it off. \302\240You wouldn't want to\n" "also set apste to zero too and call a nvme_set_features() to \"disable\n" "APST\"?\n" "\n" "I guess I'm a little confused on the error statement, \"disabling APST\",\n" "when it doesn't seem like anything is being done to actually disable\n" "APST, it's just more of an invalid state retrieved from the HW.\n" - "?\n" + "\302\240\n" "\n" "> +\t\treturn;\n" "> +\t}\n" @@ -120,14 +126,14 @@ "> +\t\tint state;\n" "> +\n" "> +\t\t/*\n" - "> +\t\t?* Walk through all states from lowest- to highest-\n" + "> +\t\t\302\240* Walk through all states from lowest- to highest-\n" "> power.\n" - "> +\t\t?* According to the spec, lower-numbered states use\n" + "> +\t\t\302\240* According to the spec, lower-numbered states use\n" "> more\n" - "> +\t\t?* power.??NPSS, despite the name, is the index of\n" + "> +\t\t\302\240* power.\302\240\302\240NPSS, despite the name, is the index of\n" "> the\n" - "> +\t\t?* lowest-power state, not the number of states.\n" - "> +\t\t?*/\n" + "> +\t\t\302\240* lowest-power state, not the number of states.\n" + "> +\t\t\302\240*/\n" "> +\t\tfor (state = (int)ctrl->npss; state >= 0; state--) {\n" "> +\t\t\tu64 total_latency_us, transition_ms;\n" "> +\n" @@ -135,13 +141,13 @@ "> +\t\t\t\ttable->entries[state] = target;\n" "> +\n" "> +\t\t\t/*\n" - "> +\t\t\t?* Is this state a useful non-operational\n" + "> +\t\t\t\302\240* Is this state a useful non-operational\n" "> state for\n" - "> +\t\t\t?* higher-power states to autonomously\n" + "> +\t\t\t\302\240* higher-power states to autonomously\n" "> transition to?\n" - "> +\t\t\t?*/\n" + "> +\t\t\t\302\240*/\n" "> +\t\t\tif (!(ctrl->psd[state].flags & 2))\n" - "> +\t\t\t\tcontinue;??/* It's an operational\n" + "> +\t\t\t\tcontinue;\302\240\302\240/* It's an operational\n" "> state. */\n" "> +\n" "> +\t\t\ttotal_latency_us =\n" @@ -154,10 +160,10 @@ "> +\t\t\t\tcontinue;\n" "> +\n" "> +\t\t\t/*\n" - "> +\t\t\t?* This state is good.??Use it as the APST\n" + "> +\t\t\t\302\240* This state is good.\302\240\302\240Use it as the APST\n" "> idle\n" - "> +\t\t\t?* target for higher power states.\n" - "> +\t\t\t?*/\n" + "> +\t\t\t\302\240* target for higher power states.\n" + "> +\t\t\t\302\240*/\n" "> +\t\t\ttransition_ms = total_latency_us + 19;\n" "> +\t\t\tdo_div(transition_ms, 20);\n" "> +\t\t\tif (transition_ms >= (1 << 24))\n" @@ -168,7 +174,7 @@ "\n" "> +\n" "> +\t\t\ttarget = cpu_to_le64((state << 3) |\n" - "> +\t\t\t\t\t?????(transition_ms << 8));\n" + "> +\t\t\t\t\t\302\240\302\240\302\240\302\240\302\240(transition_ms << 8));\n" "> +\t\t}\n" "> \n" "\n" @@ -178,12 +184,12 @@ ".\n" "\n" "> +\t/*\n" - "> +\t?* By default, allow up to 25ms of APST-induced\n" - "> latency.??This will\n" - "> +\t?* have no effect on non-APST supporting controllers (i.e.\n" + "> +\t\302\240* By default, allow up to 25ms of APST-induced\n" + "> latency.\302\240\302\240This will\n" + "> +\t\302\240* have no effect on non-APST supporting controllers (i.e.\n" "> any\n" - "> +\t?* controller with APSTA == 0).\n" - "> +\t?*/\n" + "> +\t\302\240* controller with APSTA == 0).\n" + "> +\t\302\240*/\n" "> +\tctrl->apst_max_latency_ns = 25000000;\n" "\n" "Is it possible to make that a #define please?\n" @@ -191,6 +197,6 @@ "Nice stuff!\n" "\n" "\n" - >? + > -be0041651ff79b6ced4d5e07922055eb273cb4456a32e6a810708a016551d657 +fb2e97f270c5847729257838c6ad3c2092e2d9fd1b9e545a89e9c39528abeef7
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.