linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
@ 2025-07-12  3:08 Zihuan Zhang
  2025-07-14  8:44 ` Zihuan Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Zihuan Zhang @ 2025-07-12  3:08 UTC (permalink / raw)
  To: Rafael J . Wysocki, Christian Brauner
  Cc: Len Brown, Pavel Machek, linux-pm, linux-kernel, Zihuan Zhang

The recently introduced support for freezing filesystems during system
suspend included calls to filesystems_freeze() in both suspend_prepare()
and enter_state(), as well as calls to filesystems_thaw() in both
suspend_finish() and the Unlock path in enter_state(). These are
redundant.

- filesystems_freeze() is already called in suspend_prepare(), which is
  the proper and consistent place to handle pre-suspend operations. The
second call in enter_state() is unnecessary and removed.

- filesystems_thaw() is invoked in suspend_finish(), which covers
  successful suspend/resume paths. In the failure case , we add a call
to filesystems_thaw() only when needed, avoiding the duplicate call in
the general Unlock path.

This change simplifies the suspend code and avoids repeated freeze/thaw
calls, while preserving correct ordering and behavior.

Fixes: eacfbf74196f91e4c26d9f8c78e1576c1225cd8c ("power: freeze filesystems during suspend/resume")
Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
---
 kernel/power/suspend.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index bb608b68fb30..8f3e4c48d5cd 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -384,6 +384,7 @@ static int suspend_prepare(suspend_state_t state)
 		return 0;
 
 	dpm_save_failed_step(SUSPEND_FREEZE);
+	filesystems_thaw();
 	pm_notifier_call_chain(PM_POST_SUSPEND);
  Restore:
 	pm_restore_console();
@@ -593,8 +594,6 @@ static int enter_state(suspend_state_t state)
 		ksys_sync_helper();
 		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
 	}
-	if (filesystem_freeze_enabled)
-		filesystems_freeze();
 
 	pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
 	pm_suspend_clear_flags();
@@ -614,7 +613,6 @@ static int enter_state(suspend_state_t state)
 	pm_pr_dbg("Finishing wakeup.\n");
 	suspend_finish();
  Unlock:
-	filesystems_thaw();
 	mutex_unlock(&system_transition_mutex);
 	return error;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-12  3:08 [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling Zihuan Zhang
@ 2025-07-14  8:44 ` Zihuan Zhang
  2025-07-14 17:57   ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: Zihuan Zhang @ 2025-07-14  8:44 UTC (permalink / raw)
  To: Rafael J . Wysocki, Christian Brauner
  Cc: Len Brown, Pavel Machek, linux-pm, linux-kernel

Hi Rafael,

Just a gentle ping on this patch.

I realized I forgot to mention an important motivation in the changelog:
calling filesystems_freeze() twice (from both suspend_prepare() and 
enter_state()) lead to a black screen and make the system unable to resume..

This patch avoids the duplicate call and resolves that issue.

在 2025/7/12 11:08, Zihuan Zhang 写道:
> The recently introduced support for freezing filesystems during system
> suspend included calls to filesystems_freeze() in both suspend_prepare()
> and enter_state(), as well as calls to filesystems_thaw() in both
> suspend_finish() and the Unlock path in enter_state(). These are
> redundant.
>
> - filesystems_freeze() is already called in suspend_prepare(), which is
>    the proper and consistent place to handle pre-suspend operations. The
> second call in enter_state() is unnecessary and removed.
>
> - filesystems_thaw() is invoked in suspend_finish(), which covers
>    successful suspend/resume paths. In the failure case , we add a call
> to filesystems_thaw() only when needed, avoiding the duplicate call in
> the general Unlock path.
>
> This change simplifies the suspend code and avoids repeated freeze/thaw
> calls, while preserving correct ordering and behavior.
>
> Fixes: eacfbf74196f91e4c26d9f8c78e1576c1225cd8c ("power: freeze filesystems during suspend/resume")
> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> ---
>   kernel/power/suspend.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index bb608b68fb30..8f3e4c48d5cd 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -384,6 +384,7 @@ static int suspend_prepare(suspend_state_t state)
>   		return 0;
>   
>   	dpm_save_failed_step(SUSPEND_FREEZE);
> +	filesystems_thaw();
>   	pm_notifier_call_chain(PM_POST_SUSPEND);
>    Restore:
>   	pm_restore_console();
> @@ -593,8 +594,6 @@ static int enter_state(suspend_state_t state)
>   		ksys_sync_helper();
>   		trace_suspend_resume(TPS("sync_filesystems"), 0, false);
>   	}
> -	if (filesystem_freeze_enabled)
> -		filesystems_freeze();
>   
>   	pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
>   	pm_suspend_clear_flags();
> @@ -614,7 +613,6 @@ static int enter_state(suspend_state_t state)
>   	pm_pr_dbg("Finishing wakeup.\n");
>   	suspend_finish();
>    Unlock:
> -	filesystems_thaw();
>   	mutex_unlock(&system_transition_mutex);
>   	return error;
>   }
Thanks,
Zihuan Zhang

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-14  8:44 ` Zihuan Zhang
@ 2025-07-14 17:57   ` Rafael J. Wysocki
  2025-07-15  6:12     ` Zihuan Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2025-07-14 17:57 UTC (permalink / raw)
  To: Zihuan Zhang
  Cc: Rafael J . Wysocki, Christian Brauner, Len Brown, Pavel Machek,
	linux-pm, linux-kernel

Hi,

On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
> Hi Rafael,
>
> Just a gentle ping on this patch.

I've lost track of it for some reason, sorry.

> I realized I forgot to mention an important motivation in the changelog:
> calling filesystems_freeze() twice (from both suspend_prepare() and
> enter_state()) lead to a black screen and make the system unable to resume..
>
> This patch avoids the duplicate call and resolves that issue.

Now applied as a fix for 6.16-rc7, thank you!

> 在 2025/7/12 11:08, Zihuan Zhang 写道:
> > The recently introduced support for freezing filesystems during system
> > suspend included calls to filesystems_freeze() in both suspend_prepare()
> > and enter_state(), as well as calls to filesystems_thaw() in both
> > suspend_finish() and the Unlock path in enter_state(). These are
> > redundant.
> >
> > - filesystems_freeze() is already called in suspend_prepare(), which is
> >    the proper and consistent place to handle pre-suspend operations. The
> > second call in enter_state() is unnecessary and removed.
> >
> > - filesystems_thaw() is invoked in suspend_finish(), which covers
> >    successful suspend/resume paths. In the failure case , we add a call
> > to filesystems_thaw() only when needed, avoiding the duplicate call in
> > the general Unlock path.
> >
> > This change simplifies the suspend code and avoids repeated freeze/thaw
> > calls, while preserving correct ordering and behavior.
> >
> > Fixes: eacfbf74196f91e4c26d9f8c78e1576c1225cd8c ("power: freeze filesystems during suspend/resume")
> > Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
> > ---
> >   kernel/power/suspend.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index bb608b68fb30..8f3e4c48d5cd 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -384,6 +384,7 @@ static int suspend_prepare(suspend_state_t state)
> >               return 0;
> >
> >       dpm_save_failed_step(SUSPEND_FREEZE);
> > +     filesystems_thaw();
> >       pm_notifier_call_chain(PM_POST_SUSPEND);
> >    Restore:
> >       pm_restore_console();
> > @@ -593,8 +594,6 @@ static int enter_state(suspend_state_t state)
> >               ksys_sync_helper();
> >               trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> >       }
> > -     if (filesystem_freeze_enabled)
> > -             filesystems_freeze();
> >
> >       pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> >       pm_suspend_clear_flags();
> > @@ -614,7 +613,6 @@ static int enter_state(suspend_state_t state)
> >       pm_pr_dbg("Finishing wakeup.\n");
> >       suspend_finish();
> >    Unlock:
> > -     filesystems_thaw();
> >       mutex_unlock(&system_transition_mutex);
> >       return error;
> >   }
> Thanks,
> Zihuan Zhang

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-14 17:57   ` Rafael J. Wysocki
@ 2025-07-15  6:12     ` Zihuan Zhang
  2025-07-15 12:48       ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: Zihuan Zhang @ 2025-07-15  6:12 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Christian Brauner, Len Brown, Pavel Machek, linux-pm,
	linux-kernel

Hi Rafael,

在 2025/7/15 01:57, Rafael J. Wysocki 写道:
> Hi,
>
> On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>> Hi Rafael,
>>
>> Just a gentle ping on this patch.
> I've lost track of it for some reason, sorry.
>
>> I realized I forgot to mention an important motivation in the changelog:
>> calling filesystems_freeze() twice (from both suspend_prepare() and
>> enter_state()) lead to a black screen and make the system unable to resume..
>>
>> This patch avoids the duplicate call and resolves that issue.
> Now applied as a fix for 6.16-rc7, thank you!


Thanks for the reply!

Just a quick follow-up question — we noticed that even when the “freeze 
filesystems” feature is not enabled, the current code still calls 
filesystems_thaw().

Do you think it would make sense to guard this with a static key (or 
another mechanism) to avoid unnecessary overhead?



>> 在 2025/7/12 11:08, Zihuan Zhang 写道:
>>> The recently introduced support for freezing filesystems during system
>>> suspend included calls to filesystems_freeze() in both suspend_prepare()
>>> and enter_state(), as well as calls to filesystems_thaw() in both
>>> suspend_finish() and the Unlock path in enter_state(). These are
>>> redundant.
>>>
>>> - filesystems_freeze() is already called in suspend_prepare(), which is
>>>     the proper and consistent place to handle pre-suspend operations. The
>>> second call in enter_state() is unnecessary and removed.
>>>
>>> - filesystems_thaw() is invoked in suspend_finish(), which covers
>>>     successful suspend/resume paths. In the failure case , we add a call
>>> to filesystems_thaw() only when needed, avoiding the duplicate call in
>>> the general Unlock path.
>>>
>>> This change simplifies the suspend code and avoids repeated freeze/thaw
>>> calls, while preserving correct ordering and behavior.
>>>
>>> Fixes: eacfbf74196f91e4c26d9f8c78e1576c1225cd8c ("power: freeze filesystems during suspend/resume")
>>> Signed-off-by: Zihuan Zhang <zhangzihuan@kylinos.cn>
>>> ---
>>>    kernel/power/suspend.c | 4 +---
>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>>> index bb608b68fb30..8f3e4c48d5cd 100644
>>> --- a/kernel/power/suspend.c
>>> +++ b/kernel/power/suspend.c
>>> @@ -384,6 +384,7 @@ static int suspend_prepare(suspend_state_t state)
>>>                return 0;
>>>
>>>        dpm_save_failed_step(SUSPEND_FREEZE);
>>> +     filesystems_thaw();
>>>        pm_notifier_call_chain(PM_POST_SUSPEND);
>>>     Restore:
>>>        pm_restore_console();
>>> @@ -593,8 +594,6 @@ static int enter_state(suspend_state_t state)
>>>                ksys_sync_helper();
>>>                trace_suspend_resume(TPS("sync_filesystems"), 0, false);
>>>        }
>>> -     if (filesystem_freeze_enabled)
>>> -             filesystems_freeze();
>>>
>>>        pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
>>>        pm_suspend_clear_flags();
>>> @@ -614,7 +613,6 @@ static int enter_state(suspend_state_t state)
>>>        pm_pr_dbg("Finishing wakeup.\n");
>>>        suspend_finish();
>>>     Unlock:
>>> -     filesystems_thaw();
>>>        mutex_unlock(&system_transition_mutex);
>>>        return error;
>>>    }
>> Thanks,
>> Zihuan Zhang
Best regards,
Zihuan Zhang

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-15  6:12     ` Zihuan Zhang
@ 2025-07-15 12:48       ` Rafael J. Wysocki
  2025-07-16  2:04         ` Zihuan Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2025-07-15 12:48 UTC (permalink / raw)
  To: Zihuan Zhang
  Cc: Rafael J. Wysocki, Christian Brauner, Len Brown, Pavel Machek,
	linux-pm, linux-kernel

On Tue, Jul 15, 2025 at 8:12 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
> Hi Rafael,
>
> 在 2025/7/15 01:57, Rafael J. Wysocki 写道:
> > Hi,
> >
> > On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >> Hi Rafael,
> >>
> >> Just a gentle ping on this patch.
> > I've lost track of it for some reason, sorry.
> >
> >> I realized I forgot to mention an important motivation in the changelog:
> >> calling filesystems_freeze() twice (from both suspend_prepare() and
> >> enter_state()) lead to a black screen and make the system unable to resume..
> >>
> >> This patch avoids the duplicate call and resolves that issue.
> > Now applied as a fix for 6.16-rc7, thank you!
>
>
> Thanks for the reply!
>
> Just a quick follow-up question — we noticed that even when the “freeze
> filesystems” feature is not enabled, the current code still calls
> filesystems_thaw().
>
> Do you think it would make sense to guard this with a static key (or
> another mechanism) to avoid unnecessary overhead?

Possibly, if this overhead is significant, but is it?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-15 12:48       ` Rafael J. Wysocki
@ 2025-07-16  2:04         ` Zihuan Zhang
  2025-07-16 12:23           ` Rafael J. Wysocki
  0 siblings, 1 reply; 8+ messages in thread
From: Zihuan Zhang @ 2025-07-16  2:04 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Christian Brauner, Len Brown, Pavel Machek, linux-pm,
	linux-kernel


在 2025/7/15 20:48, Rafael J. Wysocki 写道:
> On Tue, Jul 15, 2025 at 8:12 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>> Hi Rafael,
>>
>> 在 2025/7/15 01:57, Rafael J. Wysocki 写道:
>>> Hi,
>>>
>>> On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>>> Hi Rafael,
>>>>
>>>> Just a gentle ping on this patch.
>>> I've lost track of it for some reason, sorry.
>>>
>>>> I realized I forgot to mention an important motivation in the changelog:
>>>> calling filesystems_freeze() twice (from both suspend_prepare() and
>>>> enter_state()) lead to a black screen and make the system unable to resume..
>>>>
>>>> This patch avoids the duplicate call and resolves that issue.
>>> Now applied as a fix for 6.16-rc7, thank you!
>>
>> Thanks for the reply!
>>
>> Just a quick follow-up question — we noticed that even when the “freeze
>> filesystems” feature is not enabled, the current code still calls
>> filesystems_thaw().
>>
>> Do you think it would make sense to guard this with a static key (or
>> another mechanism) to avoid unnecessary overhead?
> Possibly, if this overhead is significant, but is it?

We've done some testing using ftrace to measure the overhead of 
filesystems_thaw(). When freeze_filesystems is not enabled, the overhead 
is typically around 15–40 microseconds.

However, when freeze is enabled, we observed that filesystems_thaw() can 
take over 3 seconds to complete (e.g., 3,450,644 us in one test case).

freeze_filesystems  not enabled:

# tracer: function_graph
#
# CPU  DURATION                  FUNCTION CALLS
# |     |   |                     |   |   |   |
   4) + 15.740 us   |  filesystems_thaw();
  11) + 16.894 us   |  filesystems_thaw();
  10) + 17.805 us   |  filesystems_thaw();
   8) + 37.762 us   |  filesystems_thaw();
  ------------------------------------------
  11) systemd-54512  => systemd-66433
  ------------------------------------------

  11) + 15.167 us   |  filesystems_thaw();
   6) + 16.760 us   |  filesystems_thaw();
   7) + 14.870 us   |  filesystems_thaw();
   3) + 16.171 us   |  filesystems_thaw();
   1) + 16.461 us   |  filesystems_thaw();
  ------------------------------------------
   3) systemd-71984  => systemd-73036
  ------------------------------------------

   3) + 28.314 us   |  filesystems_thaw();

freeze_filesystems  enabled:

  10)               |  filesystems_thaw() {
   2) $ 3450644 us  |  } /* filesystems_thaw */
  ------------------------------------------
   1) systemd-72561  => systemd-99210
  ------------------------------------------

   1)               |  filesystems_thaw() {
  ------------------------------------------
   7) systemd-71501  => systemd-99210
  ------------------------------------------

   7) $ 3429306 us  |  } /* filesystems_thaw */
  ------------------------------------------
   7) systemd-99210  => systemd-100028
  ------------------------------------------

   7)               |  filesystems_thaw() {
  ------------------------------------------
   4) systemd-53278  => systemd-100028
  ------------------------------------------

   4) $ 3270122 us  |  } /* filesystems_thaw */
  ------------------------------------------
   7) systemd-100028 => systemd-100720
  ------------------------------------------

   7) $ 3446496 us  |  filesystems_thaw();
  ------------------------------------------
   7) systemd-100720 => systemd-112075
  ------------------------------------------

   7)               |  filesystems_thaw() {
  ------------------------------------------
  11) systemd-66433  => systemd-112075
  ------------------------------------------

  11) $ 3454117 us  |  } /* filesystems_thaw */



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-16  2:04         ` Zihuan Zhang
@ 2025-07-16 12:23           ` Rafael J. Wysocki
  2025-07-17  0:37             ` Zihuan Zhang
  0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2025-07-16 12:23 UTC (permalink / raw)
  To: Zihuan Zhang
  Cc: Rafael J. Wysocki, Christian Brauner, Len Brown, Pavel Machek,
	linux-pm, linux-kernel

On Wed, Jul 16, 2025 at 4:04 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>
>
> 在 2025/7/15 20:48, Rafael J. Wysocki 写道:
> > On Tue, Jul 15, 2025 at 8:12 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >> Hi Rafael,
> >>
> >> 在 2025/7/15 01:57, Rafael J. Wysocki 写道:
> >>> Hi,
> >>>
> >>> On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
> >>>> Hi Rafael,
> >>>>
> >>>> Just a gentle ping on this patch.
> >>> I've lost track of it for some reason, sorry.
> >>>
> >>>> I realized I forgot to mention an important motivation in the changelog:
> >>>> calling filesystems_freeze() twice (from both suspend_prepare() and
> >>>> enter_state()) lead to a black screen and make the system unable to resume..
> >>>>
> >>>> This patch avoids the duplicate call and resolves that issue.
> >>> Now applied as a fix for 6.16-rc7, thank you!
> >>
> >> Thanks for the reply!
> >>
> >> Just a quick follow-up question — we noticed that even when the “freeze
> >> filesystems” feature is not enabled, the current code still calls
> >> filesystems_thaw().
> >>
> >> Do you think it would make sense to guard this with a static key (or
> >> another mechanism) to avoid unnecessary overhead?
> > Possibly, if this overhead is significant, but is it?
>
> We've done some testing using ftrace to measure the overhead of
> filesystems_thaw(). When freeze_filesystems is not enabled, the overhead
> is typically around 15–40 microseconds.

So this is the time that can be saved by adding a
filesystem_freeze_enabled check before calling filesystems_thaw()
IIUC.

I'd say don't bother.


> However, when freeze is enabled, we observed that filesystems_thaw() can
> take over 3 seconds to complete (e.g., 3,450,644 us in one test case).
>
> freeze_filesystems  not enabled:
>
> # tracer: function_graph
> #
> # CPU  DURATION                  FUNCTION CALLS
> # |     |   |                     |   |   |   |
>    4) + 15.740 us   |  filesystems_thaw();
>   11) + 16.894 us   |  filesystems_thaw();
>   10) + 17.805 us   |  filesystems_thaw();
>    8) + 37.762 us   |  filesystems_thaw();
>   ------------------------------------------
>   11) systemd-54512  => systemd-66433
>   ------------------------------------------
>
>   11) + 15.167 us   |  filesystems_thaw();
>    6) + 16.760 us   |  filesystems_thaw();
>    7) + 14.870 us   |  filesystems_thaw();
>    3) + 16.171 us   |  filesystems_thaw();
>    1) + 16.461 us   |  filesystems_thaw();
>   ------------------------------------------
>    3) systemd-71984  => systemd-73036
>   ------------------------------------------
>
>    3) + 28.314 us   |  filesystems_thaw();
>
> freeze_filesystems  enabled:
>
>   10)               |  filesystems_thaw() {
>    2) $ 3450644 us  |  } /* filesystems_thaw */
>   ------------------------------------------
>    1) systemd-72561  => systemd-99210
>   ------------------------------------------
>
>    1)               |  filesystems_thaw() {
>   ------------------------------------------
>    7) systemd-71501  => systemd-99210
>   ------------------------------------------
>
>    7) $ 3429306 us  |  } /* filesystems_thaw */
>   ------------------------------------------
>    7) systemd-99210  => systemd-100028
>   ------------------------------------------
>
>    7)               |  filesystems_thaw() {
>   ------------------------------------------
>    4) systemd-53278  => systemd-100028
>   ------------------------------------------
>
>    4) $ 3270122 us  |  } /* filesystems_thaw */
>   ------------------------------------------
>    7) systemd-100028 => systemd-100720
>   ------------------------------------------
>
>    7) $ 3446496 us  |  filesystems_thaw();
>   ------------------------------------------
>    7) systemd-100720 => systemd-112075
>   ------------------------------------------
>
>    7)               |  filesystems_thaw() {
>   ------------------------------------------
>   11) systemd-66433  => systemd-112075
>   ------------------------------------------
>
>   11) $ 3454117 us  |  } /* filesystems_thaw */
>
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling
  2025-07-16 12:23           ` Rafael J. Wysocki
@ 2025-07-17  0:37             ` Zihuan Zhang
  0 siblings, 0 replies; 8+ messages in thread
From: Zihuan Zhang @ 2025-07-17  0:37 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Christian Brauner, Len Brown, Pavel Machek, linux-pm,
	linux-kernel


在 2025/7/16 20:23, Rafael J. Wysocki 写道:
> On Wed, Jul 16, 2025 at 4:04 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>
>> 在 2025/7/15 20:48, Rafael J. Wysocki 写道:
>>> On Tue, Jul 15, 2025 at 8:12 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>>> Hi Rafael,
>>>>
>>>> 在 2025/7/15 01:57, Rafael J. Wysocki 写道:
>>>>> Hi,
>>>>>
>>>>> On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@kylinos.cn> wrote:
>>>>>> Hi Rafael,
>>>>>>
>>>>>> Just a gentle ping on this patch.
>>>>> I've lost track of it for some reason, sorry.
>>>>>
>>>>>> I realized I forgot to mention an important motivation in the changelog:
>>>>>> calling filesystems_freeze() twice (from both suspend_prepare() and
>>>>>> enter_state()) lead to a black screen and make the system unable to resume..
>>>>>>
>>>>>> This patch avoids the duplicate call and resolves that issue.
>>>>> Now applied as a fix for 6.16-rc7, thank you!
>>>> Thanks for the reply!
>>>>
>>>> Just a quick follow-up question — we noticed that even when the “freeze
>>>> filesystems” feature is not enabled, the current code still calls
>>>> filesystems_thaw().
>>>>
>>>> Do you think it would make sense to guard this with a static key (or
>>>> another mechanism) to avoid unnecessary overhead?
>>> Possibly, if this overhead is significant, but is it?
>> We've done some testing using ftrace to measure the overhead of
>> filesystems_thaw(). When freeze_filesystems is not enabled, the overhead
>> is typically around 15–40 microseconds.
> So this is the time that can be saved by adding a
> filesystem_freeze_enabled check before calling filesystems_thaw()
> IIUC.
>
> I'd say don't bother.
>

Understood, thanks!


>> However, when freeze is enabled, we observed that filesystems_thaw() can
>> take over 3 seconds to complete (e.g., 3,450,644 us in one test case).
>>
>> freeze_filesystems  not enabled:
>>
>> # tracer: function_graph
>> #
>> # CPU  DURATION                  FUNCTION CALLS
>> # |     |   |                     |   |   |   |
>>     4) + 15.740 us   |  filesystems_thaw();
>>    11) + 16.894 us   |  filesystems_thaw();
>>    10) + 17.805 us   |  filesystems_thaw();
>>     8) + 37.762 us   |  filesystems_thaw();
>>    ------------------------------------------
>>    11) systemd-54512  => systemd-66433
>>    ------------------------------------------
>>
>>    11) + 15.167 us   |  filesystems_thaw();
>>     6) + 16.760 us   |  filesystems_thaw();
>>     7) + 14.870 us   |  filesystems_thaw();
>>     3) + 16.171 us   |  filesystems_thaw();
>>     1) + 16.461 us   |  filesystems_thaw();
>>    ------------------------------------------
>>     3) systemd-71984  => systemd-73036
>>    ------------------------------------------
>>
>>     3) + 28.314 us   |  filesystems_thaw();
>>
>> freeze_filesystems  enabled:
>>
>>    10)               |  filesystems_thaw() {
>>     2) $ 3450644 us  |  } /* filesystems_thaw */
>>    ------------------------------------------
>>     1) systemd-72561  => systemd-99210
>>    ------------------------------------------
>>
>>     1)               |  filesystems_thaw() {
>>    ------------------------------------------
>>     7) systemd-71501  => systemd-99210
>>    ------------------------------------------
>>
>>     7) $ 3429306 us  |  } /* filesystems_thaw */
>>    ------------------------------------------
>>     7) systemd-99210  => systemd-100028
>>    ------------------------------------------
>>
>>     7)               |  filesystems_thaw() {
>>    ------------------------------------------
>>     4) systemd-53278  => systemd-100028
>>    ------------------------------------------
>>
>>     4) $ 3270122 us  |  } /* filesystems_thaw */
>>    ------------------------------------------
>>     7) systemd-100028 => systemd-100720
>>    ------------------------------------------
>>
>>     7) $ 3446496 us  |  filesystems_thaw();
>>    ------------------------------------------
>>     7) systemd-100720 => systemd-112075
>>    ------------------------------------------
>>
>>     7)               |  filesystems_thaw() {
>>    ------------------------------------------
>>    11) systemd-66433  => systemd-112075
>>    ------------------------------------------
>>
>>    11) $ 3454117 us  |  } /* filesystems_thaw */
>>
>>
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2025-07-17  0:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-12  3:08 [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling Zihuan Zhang
2025-07-14  8:44 ` Zihuan Zhang
2025-07-14 17:57   ` Rafael J. Wysocki
2025-07-15  6:12     ` Zihuan Zhang
2025-07-15 12:48       ` Rafael J. Wysocki
2025-07-16  2:04         ` Zihuan Zhang
2025-07-16 12:23           ` Rafael J. Wysocki
2025-07-17  0:37             ` Zihuan Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).