[PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Yaxiong Tian]

From: Yaxiong Tian <tianyaxiong@kylinos.cn>

This series enhances NVMe APST (Autonomous Power State Transition) support by:
1. Adding warnings for PST table allocation failures
2. Exposing APST tables via sysfs for runtime inspection
3. Providing per-controller sysfs interface for APST configuration

The changes allow better visibility and control of power management settings
through userspace tools while maintaining the existing functionality.

Yaxiong Tian (3):
  nvme: Add warning for PST table memory allocation failure in
    nvme_configure_apst
  nvme: add sysfs interface for APST table updates
  nvme: add per-controller sysfs interface for APST configuration

Changes in v2

Add mutex_lock in nvme_set_latency_tolerance() for Potential competition 
between nvme_set_latency_tolerance() and apst_update_store().

Changes in v3
In  PACH nvme: add sysfs interface for APST table updates,Add why dynamic APST 
updates are needed in the commit message, fix code formatting issues. 

 drivers/nvme/host/core.c  | 24 ++++++++++------
 drivers/nvme/host/nvme.h  |  6 ++++
 drivers/nvme/host/sysfs.c | 59 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] nvme: Add warning for PST table memory allocation failure in nvme_configure_apst

[Yaxiong Tian]

From: Yaxiong Tian <tianyaxiong@kylinos.cn>

Currently the function fails silently on PST table memory allocation failure.
Add warning messages to improve error visibility.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
---
 drivers/nvme/host/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c2d89fac86c5..fb0404fee551 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2678,8 +2678,10 @@ static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 	}
 
 	table = kzalloc(sizeof(*table), GFP_KERNEL);
-	if (!table)
+	if (!table) {
+		dev_warn(ctrl->device, "Failed to allocate pst table; not using APST\n");
 		return 0;
+	}
 
 	if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
 		/* Turn off APST. */
-- 
2.25.1

Re: [PATCH v3 1/3] nvme: Add warning for PST table memory allocation failure in nvme_configure_apst

[Christoph Hellwig]

On Tue, Apr 01, 2025 at 05:26:52PM +0800, Yaxiong Tian wrote:
> +		dev_warn(ctrl->device, "Failed to allocate pst table; not using APST\n");

Please avoid the overly long line:

		dev_warn(ctrl->device,
			"Failed to allocate pst table; not using APST\n");

Re: [PATCH v3 1/3] nvme: Add warning for PST table memory allocation failure in nvme_configure_apst

[Yaxiong Tian]

在 2025/4/3 12:25, Christoph Hellwig 写道:
> On Tue, Apr 01, 2025 at 05:26:52PM +0800, Yaxiong Tian wrote:
>> +		dev_warn(ctrl->device, "Failed to allocate pst table; not using APST\n");
> 
> Please avoid the overly long line:
> 
> 		dev_warn(ctrl->device,
> 			"Failed to allocate pst table; not using APST\n");

Thanks, I Will fix in v4.

[PATCH v3 2/3] nvme: add sysfs interface for APST table updates

[Yaxiong Tian]

From: Yaxiong Tian <tianyaxiong@kylinos.cn>

In desktop systems, users can choose between power-saving mode and
performance mode. These two modes involve different trade-offs between
NVMe performance and power efficiency, thus requiring dynamic updates to APST.

Currently, the APST (Autonomous Power State Transition) table can only be
updated during module initialization via module parameters or indirectly
by setting QoS latency requirements. This patch adds a direct sysfs
interface to allow dynamic updates to the APST table at runtime.

The new sysfs entry is created at:
/sys/class/nvme/<controller>/apst_update

This provides more flexibility in power management tuning without
requiring module reload or QoS latency changes.

Example usage:
update nvme module parameters.
echo 1  > /sys/class/nvme/nvme0/apst_update

Signed-off-by: Yaxiong Tian  <tianyaxiong@kylinos.cn>
---
 drivers/nvme/host/core.c  |  9 +++++++--
 drivers/nvme/host/nvme.h  |  2 ++
 drivers/nvme/host/sysfs.c | 24 ++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fb0404fee551..9dea1046b8b4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2654,7 +2654,7 @@ static bool nvme_apst_get_transition_time(u64 total_latency,
  *
  * Users can set ps_max_latency_us to zero to turn off APST.
  */
-static int nvme_configure_apst(struct nvme_ctrl *ctrl)
+int nvme_configure_apst(struct nvme_ctrl *ctrl)
 {
 	struct nvme_feat_auto_pst *table;
 	unsigned apste = 0;
@@ -2778,8 +2778,11 @@ static void nvme_set_latency_tolerance(struct device *dev, s32 val)
 
 	if (ctrl->ps_max_latency_us != latency) {
 		ctrl->ps_max_latency_us = latency;
-		if (nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE)
+		if (nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE) {
+			mutex_lock(&ctrl->apst_lock);
 			nvme_configure_apst(ctrl);
+			mutex_unlock(&ctrl->apst_lock);
+		}
 	}
 }
 
@@ -4852,6 +4855,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
 	ctrl->ka_last_check_time = jiffies;
 
+	mutex_init(&ctrl->apst_lock);
+
 	BUILD_BUG_ON(NVME_DSM_MAX_RANGES * sizeof(struct nvme_dsm_range) >
 			PAGE_SIZE);
 	ctrl->discard_page = alloc_page(GFP_KERNEL);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 51e078642127..7f8e10f5bf7a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -385,6 +385,7 @@ struct nvme_ctrl {
 	key_serial_t tls_pskid;
 
 	/* Power saving configuration */
+	struct mutex apst_lock;
 	u64 ps_max_latency_us;
 	bool apst_enabled;
 
@@ -828,6 +829,7 @@ void nvme_unfreeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze(struct nvme_ctrl *ctrl);
 int nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 void nvme_start_freeze(struct nvme_ctrl *ctrl);
+int nvme_configure_apst(struct nvme_ctrl *ctrl);
 
 static inline enum req_op nvme_req_op(struct nvme_command *cmd)
 {
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 6d31226f7a4f..20146eb4c671 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -684,6 +684,29 @@ static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
 #endif
 
+static ssize_t apst_update_store(struct device *dev,
+					      struct device_attribute *attr,
+					      const char *buf, size_t size)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	bool bool_data = false;
+	int err;
+
+	err = kstrtobool(buf, &bool_data);
+
+	if (err)
+		return err;
+
+	if (bool_data && nvme_ctrl_state(ctrl) == NVME_CTRL_LIVE) {
+		mutex_lock(&ctrl->apst_lock);
+		nvme_configure_apst(ctrl);
+		mutex_unlock(&ctrl->apst_lock);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_WO(apst_update);
+
 static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reset_controller.attr,
 	&dev_attr_rescan_controller.attr,
@@ -712,6 +735,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_dhchap_ctrl_secret.attr,
 #endif
 	&dev_attr_adm_passthru_err_log_enabled.attr,
+	&dev_attr_apst_update.attr,
 	NULL
 };
 
-- 
2.25.1

[PATCH v3 3/3] nvme: add per-controller sysfs interface for APST configuration

[Yaxiong Tian]

From: Yaxiong Tian <tianyaxiong@kylinos.cn>

Currently, APST (Autonomous Power State Transition) parameters are
configured as module parameters affecting all controllers uniformly,
 This lacks flexibility for heterogeneous systems.

This patch introduces per-controller sysfs attributes under each NVMe
controller's sysfs directory:
- apst_primary_timeout_ms
- apst_secondary_timeout_ms
- apst_primary_latency_tol_us
- apst_secondary_latency_tol_us

The attributes allow runtime configuration of: Timeout values for
primary/secondary states.

The existing module parameters are retained for backward compatibility
but now only serve as default values for new controllers.

Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
---
 drivers/nvme/host/core.c  | 16 ++++++++++------
 drivers/nvme/host/nvme.h  |  4 ++++
 drivers/nvme/host/sysfs.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9dea1046b8b4..c999bd905d10 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2608,22 +2608,22 @@ static int nvme_configure_host_options(struct nvme_ctrl *ctrl)
  * timeout value is returned and the matching tolerance index (1 or 2) is
  * reported.
  */
-static bool nvme_apst_get_transition_time(u64 total_latency,
+static bool nvme_apst_get_transition_time(struct nvme_ctrl *ctrl, u64 total_latency,
 		u64 *transition_time, unsigned *last_index)
 {
-	if (total_latency <= apst_primary_latency_tol_us) {
+	if (total_latency <= ctrl->apst_primary_latency_tol_us) {
 		if (*last_index == 1)
 			return false;
 		*last_index = 1;
-		*transition_time = apst_primary_timeout_ms;
+		*transition_time = ctrl->apst_primary_timeout_ms;
 		return true;
 	}
 	if (apst_secondary_timeout_ms &&
-		total_latency <= apst_secondary_latency_tol_us) {
+		total_latency <= ctrl->apst_secondary_latency_tol_us) {
 		if (*last_index <= 2)
 			return false;
 		*last_index = 2;
-		*transition_time = apst_secondary_timeout_ms;
+		*transition_time = ctrl->apst_secondary_timeout_ms;
 		return true;
 	}
 	return false;
@@ -2728,7 +2728,7 @@ int nvme_configure_apst(struct nvme_ctrl *ctrl)
 		 * for higher power states.
 		 */
 		if (apst_primary_timeout_ms && apst_primary_latency_tol_us) {
-			if (!nvme_apst_get_transition_time(total_latency_us,
+			if (!nvme_apst_get_transition_time(ctrl, total_latency_us,
 					&transition_ms, &last_lt_index))
 				continue;
 		} else {
@@ -4856,6 +4856,10 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->ka_last_check_time = jiffies;
 
 	mutex_init(&ctrl->apst_lock);
+	ctrl->apst_primary_timeout_ms = apst_primary_timeout_ms;
+	ctrl->apst_secondary_timeout_ms = apst_secondary_timeout_ms;
+	ctrl->apst_primary_latency_tol_us = apst_primary_latency_tol_us;
+	ctrl->apst_secondary_latency_tol_us = apst_secondary_latency_tol_us;
 
 	BUILD_BUG_ON(NVME_DSM_MAX_RANGES * sizeof(struct nvme_dsm_range) >
 			PAGE_SIZE);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7f8e10f5bf7a..ed9afc3c6781 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -387,6 +387,10 @@ struct nvme_ctrl {
 	/* Power saving configuration */
 	struct mutex apst_lock;
 	u64 ps_max_latency_us;
+	u64 apst_primary_timeout_ms;
+	u64 apst_secondary_timeout_ms;
+	u64 apst_primary_latency_tol_us;
+	u64 apst_secondary_latency_tol_us;
 	bool apst_enabled;
 
 	/* PCIe only: */
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 20146eb4c671..77912e95aff8 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -684,6 +684,39 @@ static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
 #endif
 
+#define nvme_apst_show_and_store_function(field)				\
+static ssize_t field##_show(struct device *dev,		\
+			    struct device_attribute *attr, char *buf)	\
+{									\
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
+\
+	return sysfs_emit(buf, "%llu\n", ctrl->field);	\
+}									\
+									\
+static ssize_t field##_store(struct device *dev,		\
+			     struct device_attribute *attr,		\
+			     const char *buf, size_t size)		\
+{									\
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);			\
+	u64 data = 0;					\
+	int err;						\
+\
+	err = kstrtou64(buf, 0, &data);		\
+	if (err < 0)						\
+		return err;						\
+							\
+	mutex_lock(&ctrl->apst_lock);		\
+	ctrl->field = data; 	\
+	mutex_unlock(&ctrl->apst_lock);		\
+	return size;			\
+}									\
+static DEVICE_ATTR_RW(field);
+
+nvme_apst_show_and_store_function(apst_primary_timeout_ms);
+nvme_apst_show_and_store_function(apst_secondary_timeout_ms);
+nvme_apst_show_and_store_function(apst_primary_latency_tol_us);
+nvme_apst_show_and_store_function(apst_secondary_latency_tol_us);
+
 static ssize_t apst_update_store(struct device *dev,
 					      struct device_attribute *attr,
 					      const char *buf, size_t size)
@@ -735,6 +768,11 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_dhchap_ctrl_secret.attr,
 #endif
 	&dev_attr_adm_passthru_err_log_enabled.attr,
+
+	&dev_attr_apst_primary_timeout_ms.attr,
+	&dev_attr_apst_secondary_timeout_ms.attr,
+	&dev_attr_apst_primary_latency_tol_us.attr,
+	&dev_attr_apst_secondary_latency_tol_us.attr,
 	&dev_attr_apst_update.attr,
 	NULL
 };
-- 
2.25.1

Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Chaitanya Kulkarni]

On 4/1/25 02:22, Yaxiong Tian wrote:
> From: Yaxiong Tian<tianyaxiong@kylinos.cn>
>
> This series enhances NVMe APST (Autonomous Power State Transition) support by:
> 1. Adding warnings for PST table allocation failures
> 2. Exposing APST tables via sysfs for runtime inspection
> 3. Providing per-controller sysfs interface for APST configuration
>
> The changes allow better visibility and control of power management settings
> through userspace tools while maintaining the existing functionality.


For the series looks good, I believe it is tested with blktest and there are
no issues ...

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

-ck

Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Christoph Hellwig]

On Tue, Apr 01, 2025 at 05:22:06PM +0800, Yaxiong Tian wrote:
> From: Yaxiong Tian <tianyaxiong@kylinos.cn>
> 
> This series enhances NVMe APST (Autonomous Power State Transition) support by:
> 1. Adding warnings for PST table allocation failures

That looks fine.

> 2. Exposing APST tables via sysfs for runtime inspection
> 3. Providing per-controller sysfs interface for APST configuration

Who is going to use this and how?  We'll need proper tools for that,
and in general I'd prefer to have a common set of policies for APST
configurations and not random people coming up with lots of random
policies.

Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Yaxiong Tian]

在 2025/4/3 12:25, Christoph Hellwig 写道:
> On Tue, Apr 01, 2025 at 05:22:06PM +0800, Yaxiong Tian wrote:
>> From: Yaxiong Tian <tianyaxiong@kylinos.cn>
>>
>> This series enhances NVMe APST (Autonomous Power State Transition) support by:
>> 1. Adding warnings for PST table allocation failures
> 
> That looks fine.
> 
>> 2. Exposing APST tables via sysfs for runtime inspection
>> 3. Providing per-controller sysfs interface for APST configuration
> 
> Who is going to use this and how?  We'll need proper tools for that,
> and in general I'd prefer to have a common set of policies for APST
> configurations and not random people coming up with lots of random
> policies.

These two patches don't fundamentally change the APST configuration 
policy, but rather enable users to configure various APST parameters in 
real-time across different devices. As mentioned in commit 
<ebd8a93aa4f5> ("nvme: extend and modify the APST configuration algorithm"):

1）This patch only introduces partial functionality from the Windows 
driver - it doesn't enable dynamic regeneration of the APST table when 
switching between AC power and battery power.

2) Additionally, using default configurations on certain brand SSDs has 
been observed to increase power consumption.

Therefore, when a system contains multiple drives and users require 
different configurations for AC vs battery power scenarios, real-time 
APST configuration updates across different devices become necessary.It 
must be said that configuring these parameters is difficult for average 
users, but they can still use the original default settings. However, 
for advanced users looking to optimize their devices, this provides them 
with the necessary interface.

As for providing suitable tools for users, I suppose every advanced user 
has their own preferences and approaches. For example, one could trace 
device idle time statistics through trace events (e.g., nvme_complete_rq 
→ nvme_setup_cmd) and use that data as a reference to configure 
apst_primary_timeout_ms.

For example，The device idle time distribution under specific operating
conditions is shown below (unit: nanoseconds):
We can set apst_primary_timeout_ms to 32ms, which ensures that over 99% 
of NVMe commands remain unaffected by APST.

@intervals:
[512, 1K)            749 |@@@@@@@@ 
    |
[1K, 2K)            4856 
|@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
[2K, 4K)            1002 |@@@@@@@@@@ 
      |
[4K, 8K)             756 |@@@@@@@@ 
      |
[8K, 16K)            413 |@@@@ 
      |
[16K, 32K)            36 | 
      |
[32K, 64K)            14 | 
      |
[64K, 128K)            9 | 
      |
[128K, 256K)          13 | 
      |
[256K, 512K)           8 | 
      |
[512K, 1M)             4 | 
      |
[1M, 2M)               4 | 
      |
[2M, 4M)               3 | 
      |
[4M, 8M)               4 | 
      |
[8M, 16M)              3 | 
      |
[16M, 32M)             0 | 
      |
[32M, 64M)             6 | 
      |
[64M, 128M)            5 | 
      |
[128M, 256M)           5 | 
      |
[256M, 512M)          15 | 
      |
[512M, 1G)             7 | 
      |

Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Christoph Hellwig]

On Thu, Apr 03, 2025 at 03:05:37PM +0800, Yaxiong Tian wrote:
> These two patches don't fundamentally change the APST configuration policy, 
> but rather enable users to configure various APST parameters in real-time 
> across different devices. As mentioned in commit <ebd8a93aa4f5> ("nvme: 
> extend and modify the APST configuration algorithm"):

And who are those users?  What tools do they use.  What settings do they
set and why?

RE: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Alexey Bogoslavsky]

Hi Yaxiong Tian,
The ability to configure the APST-related thresholds is not the essence of the commit
you mentioned, but rather an added bonus, which I suppose not many people actually use.
The actual change is about reducing the number of non-operational states in the APST table to just two,
regardless of the number of non-operational states advertised by the device, and, more importantly,
removal of the linear dependency between the ENLAT and EXLAT and the ITPT which, before the change
was made, caused an unreasonably high ITPT in devices with high latencies, which, in turn, prevented
transitions to non-operational states and led to an unjustified waste of power. The modified code
was tested thoroughly and showed significant improvement for some devices and only a minor negative effect 
for others. All results appear in the commit's description.

Thanks,
Alexey

-----Original Message-----
From: Christoph Hellwig <hch@lst.de> 
Sent: Friday, April 4, 2025 11:30 AM
To: Yaxiong Tian <iambestgod@qq.com>
Cc: Christoph Hellwig <hch@lst.de>; kbusch@kernel.org; axboe@kernel.dk; sagi@grimberg.me; chaitanyak@nvidia.com; linux-nvme@lists.infradead.org; linux-kernel@vger.kernel.org; Yaxiong Tian <tianyaxiong@kylinos.cn>; alexey.bogoslavsky@wdc.com
Subject: Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

On Thu, Apr 03, 2025 at 03:05:37PM +0800, Yaxiong Tian wrote:
> These two patches don't fundamentally change the APST configuration 
> policy, but rather enable users to configure various APST parameters 
> in real-time across different devices. As mentioned in commit <ebd8a93aa4f5> ("nvme:
> extend and modify the APST configuration algorithm"):

And who are those users?  What tools do they use.  What settings do they set and why?

Re: [PATCH v3 0/3] nvme: Add sysfs interface for APST configuration management

[Yaxiong Tian]

在 2025/4/4 16:29, Christoph Hellwig 写道:
> On Thu, Apr 03, 2025 at 03:05:37PM +0800, Yaxiong Tian wrote:
>> These two patches don't fundamentally change the APST configuration policy,
>> but rather enable users to configure various APST parameters in real-time
>> across different devices. As mentioned in commit <ebd8a93aa4f5> ("nvme:
>> extend and modify the APST configuration algorithm"):
> 
> And who are those users?  What tools do they use.  What settings do they
> set and why?
The three issues you mentioned were actually partially addressed in the 
previous email, or at least touched upon.

In any case, I agree with applying a universal setting for APST, as I 
don’t have a good tool to handle this either Now.