Skip to content

[暂不合并]feat(fake-wm): add dde-thp-disable pre-start script to service#269

Open
mhduiy wants to merge 1 commit into
linuxdeepin:masterfrom
mhduiy:thp
Open

[暂不合并]feat(fake-wm): add dde-thp-disable pre-start script to service#269
mhduiy wants to merge 1 commit into
linuxdeepin:masterfrom
mhduiy:thp

Conversation

@mhduiy
Copy link
Copy Markdown
Contributor

@mhduiy mhduiy commented May 25, 2026

  1. Added ExecStartPre hook to run dde-thp-disable before dde-fakewm starts
  2. The pre-start script ignores failure to avoid blocking service startup

Log: add dde-thp-disable pre-start script to dde-fakewm service

feat(fake-wm): 在服务中新增 dde-thp-disable 预启动脚本

  1. 添加了 ExecStartPre 钩子,在 dde-fakewm 启动前运行 dde-thp-disable
  2. 预启动脚本忽略执行失败,避免阻塞服务启动

Log: 在 dde-fakewm 服务中新增 dde-thp-disable 预启动脚本
PMS: TASK-390043

Summary by Sourcery

Enhancements:

  • Run dde-thp-disable as an ExecStartPre step for the dde-fakewm systemd service to disable transparent huge pages before the window manager starts.

1. Added ExecStartPre hook to run dde-thp-disable before dde-fakewm starts
2. The pre-start script ignores failure to avoid blocking service startup

Log: add dde-thp-disable pre-start script to dde-fakewm service

feat(fake-wm): 在服务中新增 dde-thp-disable 预启动脚本

1. 添加了 ExecStartPre 钩子,在 dde-fakewm 启动前运行 dde-thp-disable
2. 预启动脚本忽略执行失败,避免阻塞服务启动

Log: 在 dde-fakewm 服务中新增 dde-thp-disable 预启动脚本
PMS: TASK-390043
@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mhduiy
Copy link
Copy Markdown
Contributor Author

mhduiy commented May 25, 2026

@sourcery-ai
Copy link
Copy Markdown

sourcery-ai Bot commented May 25, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds a pre-start ExecStartPre hook to the dde-fakewm systemd service to run dde-thp-disable before the main process, while allowing failures without blocking service startup.

File-Level Changes

Change Details Files
Run dde-thp-disable as a pre-start hook for the dde-fakewm systemd service without failing the service on errors.
  • Add an ExecStartPre directive to invoke dde-thp-disable before dde-fakewm starts
  • Configure the pre-start command to ignore non-zero exit codes so the main service still starts if dde-thp-disable fails
misc/systemd/dde-fakewm.service.in

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的 systemd 服务单元文件的 git diff。

本次修改在 dde-fakewm.service.in 中添加了一行 ExecStartPre=-/usr/libexec/dde-thp-disable,意图在启动主服务 dde-fakewm 之前,执行一个名为 dde-thp-disable 的脚本(推测用于禁用透明大页 THP,Transparent Huge Pages)。

以下是我从语法逻辑、代码质量、代码性能和代码安全四个维度提出的审查意见和改进建议:

1. 语法与逻辑

  • 前缀 - 的正确使用:你使用了 ExecStartPre=-/usr/libexec/...,前缀 - 是正确的,它表示即使该脚本执行失败(返回非零退出码),systemd 也不会中止服务的启动流程,而是继续执行 ExecStart。这在处理像关闭 THP 这种非关键且可能受环境影响的操作时是非常合理的。
  • 执行顺序逻辑ExecStartPreExecCondition 的顺序符合预期。systemd 会先执行 ExecCondition,如果当前是 Wayland 会话(退出码为2),则直接跳过后续启动;如果是 X11 会话,则继续执行 ExecStartPreExecStart。逻辑无误。

2. 代码质量

  • 硬编码路径/usr/libexec/ 是一个硬编码的绝对路径。考虑到这是一个通过 CMake 构建的系统(文件名包含 .in 且使用了 @CMAKE_INSTALL_FULL_BINDIR@ 变量),建议保持一致性,使用 CMake 变量来替换该路径,例如使用 @CMAKE_INSTALL_FULL_LIBEXECDIR@
    • 改进建议:将 ExecStartPre=-/usr/libexec/dde-thp-disable 改为 ExecStartPre=-@CMAKE_INSTALL_FULL_LIBEXECDIR@/dde-thp-disable。这能提高跨平台和不同发行版的兼容性(例如,某些发行版遵循 FHS 规范,可能使用 /usr/libexec,而另一些如 Debian 系可能使用 /usr/lib/<arch>//usr/lib/)。
  • 可读性与注释:禁用 THP 是一个针对特定性能问题的 workaround(通常是为了避免内存碎片导致的延迟)。建议在 .service.in 文件中添加注释说明为什么需要这么做,方便未来的维护者理解。

3. 代码性能

  • 同步阻塞执行ExecStartPre 默认是同步执行的,systemd 会等待其执行完毕后才启动主进程。如果 dde-thp-disable 脚本内部执行了耗时操作(例如写入 /sys/kernel/mm/transparent_hugepage/enabled 并等待同步),可能会拖慢服务的启动速度。
    • 改进建议:确保 dde-thp-disable 脚本极其轻量,仅执行必要的文件写入操作,避免在其中调用网络请求或复杂计算。如果操作确实耗时,可考虑在脚本内部异步化,或者评估是否可以通过 systemd 的 tmpfiles.dsysctl.d 在系统启动早期更全局地处理 THP 配置,而不是在用户会话服务中处理。

4. 代码安全

  • 权限与权限提升:修改内核参数(如 THP 状态)通常需要 root 权限。而该服务使用了 Type=dbus 且位于 Slice=session.slice,这表明它是一个用户会话服务,以普通用户身份运行。如果 dde-thp-disable 内部使用了 pkexecsudo 或设置了 SUID 位来提权,这可能带来安全风险。
    • 改进建议
      1. 审查 dde-thp-disable 的权限。如果是通过 SUID root 运行的二进制文件,必须确保它对输入进行了严格校验,防止提权漏洞。
      2. 更安全的系统级做法是:如果整个桌面环境都需要禁用 THP,应该在系统级(如 /etc/tmpfiles.d//etc/systemd/system/systemd-oomd.service.d/override.conf)配置,而不是让一个用户会话服务去提权修改全局内核状态。
  • 脚本可写风险:确保 /usr/libexec/dde-thp-disable(或对应的 CMake 安装目录)的文件权限为 0755 且属主为 root,防止普通用户恶意篡改该脚本从而在服务启动时获取提权执行。

综合改进建议代码

如果你采纳了 CMake 变量和注释的建议,修改后的 diff 应该类似如下:

diff --git a/misc/systemd/dde-fakewm.service.in b/misc/systemd/dde-fakewm.service.in
index 795ddd8..762fa36 100644
--- a/misc/systemd/dde-fakewm.service.in
+++ b/misc/systemd/dde-fakewm.service.in
@@ -8,5 +8,7 @@ After=plasma-kglobalaccel.service
 Type=dbus
 BusName=com.deepin.wm
 ExecCondition=/bin/sh -c 'test "$XDG_SESSION_TYPE" != "wayland" || exit 2'
+# Disable THP to avoid memory fragmentation and latency issues in the desktop environment
+ExecStartPre=-@CMAKE_INSTALL_FULL_LIBEXECDIR@/dde-thp-disable
 ExecStart=@CMAKE_INSTALL_FULL_BINDIR@/dde-fakewm
 Slice=session.slice

总结:当前的修改在语法和逻辑上是正确的,- 前缀的使用也体现了对异常情况的容错处理。主要的改进空间在于避免硬编码路径以及审查提权操作的安全性

@mhduiy mhduiy changed the title feat(fake-wm): add dde-thp-disable pre-start script to service [暂不合并]feat(fake-wm): add dde-thp-disable pre-start script to service May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants