Skip to content

feat(thp): disable transparent huge pages for DDE daemon services#1126

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

feat(thp): disable transparent huge pages for DDE daemon services#1126
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. Add dde-thp-disable script to disable THP at cgroup level before service startup
  2. Integrate as ExecStartPre in dde-lock, dde-system-daemon, and dde-session-daemon services
  3. Update Makefile and Debian packaging to install the script to /usr/libexec/

Log: Disable THP for DDE daemon processes to optimize memory behavior

feat(thp): 禁用 DDE 守护进程的透明大页

  1. 新增 dde-thp-disable 脚本,在服务启动前通过 cgroup 禁用 THP
  2. 将脚本作为 ExecStartPre 集成到 dde-lock、dde-system-daemon 和 dde-session-daemon 服务中
  3. 更新 Makefile 和 Debian 打包规则,将脚本安装到 /usr/libexec/

Log: 为 DDE 守护进程禁用透明大页以优化内存行为
PMS: TASK-390043

Summary by Sourcery

Disable transparent huge pages for core DDE daemon services via a new helper script and service unit integration.

New Features:

  • Introduce dde-thp-disable helper script to disable transparent huge pages at the cgroup level before daemon startup.

Enhancements:

  • Wire dde-thp-disable as an ExecStartPre step in dde-lock, dde-system-daemon, and dde-session-daemon systemd units to control THP behavior for these processes.
  • Install the dde-thp-disable script into /usr/libexec via Makefile and Debian packaging updates.

1. Add dde-thp-disable script to disable THP at cgroup level before service startup
2. Integrate as ExecStartPre in dde-lock, dde-system-daemon, and dde-session-daemon services
3. Update Makefile and Debian packaging to install the script to /usr/libexec/

Log: Disable THP for DDE daemon processes to optimize memory behavior

feat(thp): 禁用 DDE 守护进程的透明大页

1. 新增 dde-thp-disable 脚本,在服务启动前通过 cgroup 禁用 THP
2. 将脚本作为 ExecStartPre 集成到 dde-lock、dde-system-daemon 和 dde-session-daemon 服务中
3. 更新 Makefile 和 Debian 打包规则,将脚本安装到 /usr/libexec/

Log: 为 DDE 守护进程禁用透明大页以优化内存行为
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

@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 helper script to disable transparent huge pages (THP) via cgroup for key DDE daemons, wires it into their systemd units as an ExecStartPre step, and updates build/packaging to install the script under /usr/libexec.

Sequence diagram for DDE daemon startup with THP disabling

sequenceDiagram
    participant systemd
    participant dde_thp_disable as dde_thp_disable
    participant cgroup
    participant dde_daemon as DDE_daemon

    systemd->>dde_thp_disable: ExecStartPre /usr/libexec/dde-thp-disable
    dde_thp_disable->>cgroup: disable THP for daemon cgroup
    dde_thp_disable-->>systemd: exit 0
    systemd->>dde_daemon: ExecStart (dde-lock / dde-system-daemon / dde-session-daemon)
Loading

File-Level Changes

Change Details Files
Introduce dde-thp-disable helper script to disable THP at cgroup level for daemon processes.
  • Add misc/thp/dde-thp-disable shell script that manipulates the relevant cgroup and kernel THP interfaces before daemon startup
  • Implement logic to be callable generically for multiple DDE daemon services
misc/thp/dde-thp-disable
Ensure dde-thp-disable is installed into the system libexec directory as part of build and packaging.
  • Create ${PREFIX}/libexec in the install target if missing
  • Install misc/thp/dde-thp-disable as an executable at ${PREFIX}/libexec/dde-thp-disable
  • Update Debian packaging rules to include the installed script in the dde-daemon package
Makefile
debian/dde-daemon.install
Hook THP disabling into DDE system and user daemons via systemd ExecStartPre.
  • Add ExecStartPre invocation of /usr/libexec/dde-thp-disable to dde-lock-service system unit
  • Add ExecStartPre invocation of /usr/libexec/dde-thp-disable to dde-system-daemon system unit
  • Add ExecStartPre invocation of /usr/libexec/dde-thp-disable to org.dde.session.Daemon1 user unit
misc/systemd/services/system/dde-lock-service.service
misc/systemd/services/system/dde-system-daemon.service
misc/systemd/services/user/org.dde.session.Daemon1.service

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 left some high level feedback:

  • Consider marking the new ExecStartPre invocation of dde-thp-disable as non-fatal (e.g., prefixing with - in the unit) so that any unexpected failure in the helper script doesn’t prevent the main daemon from starting.
  • Within dde-thp-disable, ensure you robustly handle different cgroup layouts (v1 vs v2, non-existent paths, permission issues) and clearly no-op with a successful exit code when THP cannot be adjusted for the current environment.
  • Since install -D already creates parent directories, you can simplify the Makefile install target by dropping the explicit mkdir -pv ${DESTDIR}${PREFIX}/libexec to avoid redundant directory creation.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider marking the new ExecStartPre invocation of `dde-thp-disable` as non-fatal (e.g., prefixing with `-` in the unit) so that any unexpected failure in the helper script doesn’t prevent the main daemon from starting.
- Within `dde-thp-disable`, ensure you robustly handle different cgroup layouts (v1 vs v2, non-existent paths, permission issues) and clearly no-op with a successful exit code when THP cannot be adjusted for the current environment.
- Since `install -D` already creates parent directories, you can simplify the Makefile install target by dropping the explicit `mkdir -pv ${DESTDIR}${PREFIX}/libexec` to avoid redundant directory creation.

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。我已仔细审查了你提供的Git Diff,该补丁的主要目的是在多个DDE核心服务启动前,通过systemd的ExecStartPre指令执行一个Shell脚本,以禁用特定cgroup下的透明大页,从而避免潜在的内存性能问题(如THP导致的延迟)。

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

1. 语法与逻辑

  • Shell脚本逻辑隐患 (awk + head 组合)
    脚本中 CG=$(awk -F: '$1=="0"{print $3}' /proc/self/cgroup | head -n1) 存在逻辑隐患。在cgroup v2中,/proc/self/cgroup 通常只有一行 0::/path/to/cgroup,该命令可以正常工作。但在cgroup v1中,会有多行记录,$1=="0" 匹配的是cgroup v2的层级(如果系统是混合模式),或者可能根本匹配不到(纯v1下没有层级0)。如果系统是纯cgroup v1,CG将为空,脚本会静默退出,不会报错,这可能不是预期行为。
    • 建议:明确对cgroup v2的支持(因为memory.thp_mode是cgroup v2的特性),并简化获取路径的逻辑:
      # 提取cgroup v2的路径,如果没有则退出
      CG=$(awk -F: '$1=="0" && $2=="" {print $3; exit}' /proc/self/cgroup)
  • Makefile安装逻辑不一致
    在Makefile中,install -Dm755 命令本身会自动创建不存在的目标目录。但在其上方,你保留了 mkdir -pv ${DESTDIR}${PREFIX}/libexec。虽然这不会报错,但属于冗余操作。
    • 建议:移除 mkdir -pv ${DESTDIR}${PREFIX}/libexec,仅保留 install -Dm755 ... 即可,保持代码简洁。

2. 代码质量

  • 版权年份错误
    misc/thp/dde-thp-disable 文件头部的版权年份写的是 2026,这显然是一个笔误。
    • 建议:修改为当前年份(如 2024 或项目创立年份)。
  • 硬编码路径
    在systemd service文件中,ExecStartPre=-/usr/libexec/dde-thp-disable 使用了硬编码的绝对路径。虽然/usr/libexec在Linux发行版中很常见,但不同发行版可能有不同的规范(例如某些发行版可能使用/usr/lib/deepin-daemon/)。
    • 建议:如果项目的构建系统支持变量替换(如通过configure_filesed替换@PREFIX@),建议systemd文件中使用类似 ExecStartPre=-@PREFIX@/libexec/dde-thp-disable 的占位符,以增强跨平台移植性。如果项目目前仅针对特定发行版(如deepin),则硬编码是可以接受的。
  • Bash与POSIX Shell
    脚本使用 #!/bin/bash,但脚本内容完全符合POSIX标准,没有使用任何Bash特有的特性(如数组、[[ ]]等)。
    • 建议:将Shebang改为 #!/bin/sh,这样可以提高在精简环境(如不预装bash的容器或最小化安装系统)下的兼容性,且sh执行速度略快。

3. 代码性能

  • 不必要的子进程和管道
    awk ... | head -n1 会产生两个子进程(awkhead)。在系统服务启动的关键路径上,应尽量减少子进程的创建以提升启动速度。
    • 建议awk自身就可以限制输出行数,无需借助head
      CG=$(awk -F: '$1=="0" && $2=="" {print $3; exit}' /proc/self/cgroup)
  • 文件存在性检查冗余
    if [ -e "$FILE" ]; then echo ... > "$FILE"; fi 逻辑中,先检查文件存在再写入。在并发或极端情况下,存在TOCTOU(检查时间与使用时间不一致)问题,且多了一次系统调用。
    • 建议:直接尝试写入,通过返回值判断是否成功。你已经在 echo 后面加了 2>/dev/null,所以即使文件不存在也会静默处理:
      if ! echo disable > "$FILE" 2>/dev/null; then
          logger -t thp-disable "failed to write disable to $FILE"
      fi

4. 代码安全

  • 未初始化变量与潜在的安全风险
    脚本中 [ -n "${CG:-}" ] 是一个好习惯(使用了默认值),但在后续的 FILE="/sys/fs/cgroup${CG}/memory.thp_mode" 中,如果CG包含恶意构造的路径(虽然在systemd服务环境下极难发生,但作为安全编程规范仍需注意),可能会写入意外位置。
    • 建议:对提取出的CG路径进行简单的合法性校验,确保它以 / 开头且不包含路径遍历字符:
      CG=$(awk -F: '$1=="0" && $2=="" {print $3; exit}' /proc/self/cgroup)
      # 简单校验:非空且以/开头
      if [ -z "$CG" ] || [ "${CG#/}" = "$CG" ]; then
          exit 0
      fi
  • 服务执行权限与作用域
    dde-lock-service.service 是以 lightdm 用户运行的,而 memory.thp_mode 通常需要 root 权限才能写入。虽然脚本做了 2>/dev/nulllogger 处理不会导致服务崩溃,但每次启动锁屏服务都会产生一条 "failed to write" 的错误日志,这会给运维排错带来干扰。
    • 建议
      1. 评估是否真的需要在 dde-lock-service.service(User=lightdm)中加入此指令,因为该服务大概率没有权限修改cgroup的THP设置。
      2. 如果确实需要,考虑通过systemd的 systemd-tmpfiles 或专门的systemd service(Type=oneshot)在系统启动时(root权限下)统一配置THP,而不是在每个服务启动时盲目尝试。

改进后的代码建议

1. 改进后的 misc/thp/dde-thp-disable

#!/bin/sh

# SPDX-FileCopyrightText: 2024 UnionTech Software Technology Co., Ltd.
#
# SPDX-License-Identifier: GPL-3.0-or-later

# 获取cgroup v2的路径
CG=$(awk -F: '$1=="0" && $2=="" {print $3; exit}' /proc/self/cgroup)

# 校验路径非空且为绝对路径,防止路径遍历
if [ -z "$CG" ] || [ "${CG#/}" = "$CG" ]; then
    exit 0
fi

FILE="/sys/fs/cgroup${CG}/memory.thp_mode"

# 直接尝试写入,避免TOCTOU问题,减少系统调用
if ! echo disable > "$FILE" 2>/dev/null; then
    logger -t thp-disable "failed to write disable to $FILE"
fi

exit 0

2. 改进后的 Makefile 相关片段:

install: build install-dde-data install-icons
	# 移除了冗余的 mkdir -pv ${DESTDIR}${PREFIX}/libexec
	install -Dm755 misc/thp/dde-thp-disable ${DESTDIR}${PREFIX}/libexec/dde-thp-disable

	mkdir -pv ${DESTDIR}${PREFIX}/lib/deepin-daemon
	cp -f out/bin/* ${DESTDIR}${PREFIX}/lib/deepin-daemon/

3. 关于 systemd 服务的建议:
移除 dde-lock-service.service 中的 ExecStartPre=-/usr/libexec/dde-thp-disable,因为非root用户执行此操作必然失败,属于无效且产生干扰日志的操作。保留在 dde-system-daemon.service 中的即可。

@mhduiy
Copy link
Copy Markdown
Contributor Author

mhduiy commented May 25, 2026

此 PR 暂不合并

@deepin-bot
Copy link
Copy Markdown
Contributor

deepin-bot Bot commented May 25, 2026

TAG Bot

New tag: 6.1.92
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #1127

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