[v2,1/2] alarmtimer: Create alarmtimer sysfs to make duration of kernel suspend check configurable
Commit Message
Currently, the alarmtimer_suspend does not allow the kernel
to suspend if the next alarm is within 2 seconds.
Create alarmtimer sysfs to make the value of 2 seconds configurable.
This allows flexibility to provide a different value based on the
type of device running the Linux kernel. As a data point, about 40% of
kernel suspend failures in a subset of Android devices were due to
this check. A differently configured value can avoid these suspend
failures which performs a lot of additional work affecting the
power consumption of these Android devices.
Signed-off-by: Pranav Prasad <pranavpp@google.com>
---
kernel/time/alarmtimer.c | 61 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 3 deletions(-)
Comments
On Thu, Feb 8, 2024 at 11:56 AM Pranav Prasad <pranavpp@google.com> wrote:
>
> Currently, the alarmtimer_suspend does not allow the kernel
> to suspend if the next alarm is within 2 seconds.
> Create alarmtimer sysfs to make the value of 2 seconds configurable.
> This allows flexibility to provide a different value based on the
> type of device running the Linux kernel. As a data point, about 40% of
> kernel suspend failures in a subset of Android devices were due to
> this check. A differently configured value can avoid these suspend
> failures which performs a lot of additional work affecting the
> power consumption of these Android devices.
>
> Signed-off-by: Pranav Prasad <pranavpp@google.com>
I might suggest flipping the order of these two patches, as I'm more
wary of UABI changes, so I don't want to hold up the second patch on
interface bike shedding.
> ---
> kernel/time/alarmtimer.c | 61 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index 4657cb8e8b1f..e4b88c8dc0e1 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -33,6 +33,8 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/alarmtimer.h>
>
> +static const char alarmtimer_group_name[] = "alarmtimer";
> +
> /**
> * struct alarm_base - Alarm timer bases
> * @lock: Lock for syncrhonized access to the base
> @@ -63,6 +65,56 @@ static struct rtc_timer rtctimer;
> static struct rtc_device *rtcdev;
> static DEFINE_SPINLOCK(rtcdev_lock);
>
> +/* Duration to check for soonest alarm during kernel suspend */
> +static unsigned long suspend_check_duration_ms = 2 * MSEC_PER_SEC;
Naming is hard, but "suspend_check_duration" feels particularly opaque
for a tunable knob.
I can't say I've got a better suggestion off the top of my head, but
this might be something worth thinking a bit more on.
"imminent_alarm_window" maybe? Though that's not obvious it is
connected to suspend, and maybe sounds more urgent than it should.
"suspend_alarm_pending_window"?
It might also be nice to provide some more details in the commit
message about why this should be configurable, and how a user of the
interface might choose a proper value to use (including the downsides
of going too far in either direction?).
thanks
-john
On Fri, Feb 09 2024 at 12:01, John Stultz wrote:
> On Thu, Feb 8, 2024 at 11:56 AM Pranav Prasad <pranavpp@google.com> wrote:
>>
>> Currently, the alarmtimer_suspend does not allow the kernel
>> to suspend if the next alarm is within 2 seconds.
>> Create alarmtimer sysfs to make the value of 2 seconds configurable.
>> This allows flexibility to provide a different value based on the
>> type of device running the Linux kernel. As a data point, about 40% of
>> kernel suspend failures in a subset of Android devices were due to
>> this check. A differently configured value can avoid these suspend
>> failures which performs a lot of additional work affecting the
>> power consumption of these Android devices.
>>
>> Signed-off-by: Pranav Prasad <pranavpp@google.com>
>
> I might suggest flipping the order of these two patches, as I'm more
> wary of UABI changes, so I don't want to hold up the second patch on
> interface bike shedding.
Correct. It's an orthogonal issue and an optimization on top of the
early check.
Thanks,
tglx
@@ -33,6 +33,8 @@
#define CREATE_TRACE_POINTS
#include <trace/events/alarmtimer.h>
+static const char alarmtimer_group_name[] = "alarmtimer";
+
/**
* struct alarm_base - Alarm timer bases
* @lock: Lock for syncrhonized access to the base
@@ -63,6 +65,56 @@ static struct rtc_timer rtctimer;
static struct rtc_device *rtcdev;
static DEFINE_SPINLOCK(rtcdev_lock);
+/* Duration to check for soonest alarm during kernel suspend */
+static unsigned long suspend_check_duration_ms = 2 * MSEC_PER_SEC;
+
+static ssize_t suspend_check_duration_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%lu\n", suspend_check_duration_ms);
+}
+
+static ssize_t suspend_check_duration_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t n)
+{
+ unsigned long val;
+
+ if (kstrtoul(buf, 10, &val))
+ return -EINVAL;
+
+ suspend_check_duration_ms = val;
+
+ return n;
+}
+
+static struct kobj_attribute suspend_check_duration =
+ __ATTR_RW(suspend_check_duration);
+
+static struct attribute *alarmtimer_attrs[] = {
+ &suspend_check_duration.attr,
+ NULL,
+};
+
+static const struct attribute_group alarmtimer_attr_group = {
+ .name = alarmtimer_group_name,
+ .attrs = alarmtimer_attrs,
+};
+
+/**
+ * alarmtimer_sysfs_add - Adds sysfs attributes for alarmtimer
+ *
+ * Returns 0 if successful, non-zero value for error.
+ */
+static int alarmtimer_sysfs_add(void)
+{
+ int ret = sysfs_create_group(kernel_kobj, &alarmtimer_attr_group);
+
+ if (ret)
+ pr_warn("[%s] failed to create a sysfs group\n", __func__);
+
+ return ret;
+}
+
/**
* alarmtimer_get_rtcdev - Return selected rtcdevice
*
@@ -98,8 +150,11 @@ static int alarmtimer_rtc_add_device(struct device *dev)
pdev = platform_device_register_data(dev, "alarmtimer",
PLATFORM_DEVID_AUTO, NULL, 0);
- if (!IS_ERR(pdev))
+ if (!IS_ERR(pdev)) {
device_init_wakeup(&pdev->dev, true);
+ if (alarmtimer_sysfs_add())
+ pr_warn("[%s] Failed to add alarmtimer sysfs attributes\n", __func__);
+ }
spin_lock_irqsave(&rtcdev_lock, flags);
if (!IS_ERR(pdev) && !rtcdev) {
@@ -279,8 +334,8 @@ static int alarmtimer_suspend(struct device *dev)
if (min == 0)
return 0;
- if (ktime_to_ns(min) < 2 * NSEC_PER_SEC) {
- pm_wakeup_event(dev, 2 * MSEC_PER_SEC);
+ if (ktime_to_ns(min) < suspend_check_duration_ms * NSEC_PER_MSEC) {
+ pm_wakeup_event(dev, suspend_check_duration_ms);
return -EBUSY;
}