Re: [PATCH v1] PM: suspend: clean up redundant filesystems_freeze/thaw handling

[Zihuan Zhang]

在 2025/7/16 20:23, Rafael J. Wysocki 写道:
> On Wed, Jul 16, 2025 at 4:04 AM Zihuan Zhang <zhangzihuan@xxxxxxxxxx> wrote:
> > 在 2025/7/15 20:48, Rafael J. Wysocki 写道:
> > > On Tue, Jul 15, 2025 at 8:12 AM Zihuan Zhang <zhangzihuan@xxxxxxxxxx> wrote:
> > > > Hi Rafael,
> > > >
> > > > 在 2025/7/15 01:57, Rafael J. Wysocki 写道:
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 14, 2025 at 10:44 AM Zihuan Zhang <zhangzihuan@xxxxxxxxxx> 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 */