Skip to content

[暂不合并]feat(service): disable THP before clipboard service startup#266

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

[暂不合并]feat(service): disable THP before clipboard service startup#266
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 ExecStartPre script to disable Transparent Huge Pages before dde-clipboard-daemon and dde-clipboard services start
  2. Use leading dash (-) to allow service startup even if THP disable script fails

Log: Disable THP before clipboard services start to improve performance

feat(service): 剪切板服务启动前禁用透明大页

  1. 在 dde-clipboard-daemon 和 dde-clipboard 两个服务中增加 ExecStartPre 脚本,启动前禁用透明大页
  2. 使用前置短横线 (-) 确保即使脚本失败也不影响服务正常启动

Log: 剪切板服务启动前禁用透明大页以提升性能
PMS: TASK-390043

Summary by Sourcery

Enhancements:

  • Add a pre-start step to dde-clipboard-daemon and dde-clipboard systemd services to disable Transparent Huge Pages before service startup.

1. Add ExecStartPre script to disable Transparent Huge Pages before
dde-clipboard-daemon and dde-clipboard services start
2. Use leading dash (-) to allow service startup even if THP disable
script fails

Log: Disable THP before clipboard services start to improve performance

feat(service): 剪切板服务启动前禁用透明大页

1. 在 dde-clipboard-daemon 和 dde-clipboard 两个服务中增加
ExecStartPre 脚本,启动前禁用透明大页
2. 使用前置短横线 (-) 确保即使脚本失败也不影响服务正常启动

Log: 剪切板服务启动前禁用透明大页以提升性能
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

This PR updates the systemd units for dde-clipboard-daemon and dde-clipboard to run a pre-start script that disables Transparent Huge Pages (THP) for performance, while allowing the services to continue starting even if the THP-disabling step fails.

File-Level Changes

Change Details Files
Run a pre-start THP disable script for clipboard-related systemd services without making service startup depend on its success.
  • Add an ExecStartPre command that invokes a script to disable Transparent Huge Pages before the daemon starts
  • Prefix the ExecStartPre command with a leading dash so the service still starts if the THP disable command fails
misc/dde-clipboard-daemon.service.in
Apply the same THP pre-start behavior to the main clipboard systemd service as to the daemon.
  • Add an ExecStartPre command that invokes a script to disable Transparent Huge Pages before the clipboard service starts
  • Prefix the ExecStartPre command with a leading dash so the service still starts if the THP disable command fails
misc/dde-clipboard.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 left some high level feedback:

  • Consider guarding the THP disable ExecStartPre commands with existence checks (e.g., ConditionPathExists or shell test -e around /sys/kernel/mm/transparent_hugepage/*) so the units behave cleanly on kernels or environments where THP is not exposed in the usual locations.
  • If the same THP-disabling logic is duplicated in both service files, consider centralizing it in a single script or systemd unit to avoid divergence and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider guarding the THP disable ExecStartPre commands with existence checks (e.g., ConditionPathExists or shell `test -e` around `/sys/kernel/mm/transparent_hugepage/*`) so the units behave cleanly on kernels or environments where THP is not exposed in the usual locations.
- If the same THP-disabling logic is duplicated in both service files, consider centralizing it in a single script or systemd unit to avoid divergence and make future changes easier.

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 (Deepin Desktop Environment) 剪贴板相关的 systemd 服务单元文件增加了 ExecStartPre=-/usr/libexec/dde-thp-disable 配置项。

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

1. 语法逻辑

  • 语法正确性:在 systemd 的 ExecStartPre 指令前加上 - 前缀是符合语法的。它的逻辑含义是“即使该命令执行失败(返回非零退出码),也继续执行后续的 ExecStart 操作”。这在处理可选的预处理脚本时是标准且正确的做法。
  • 执行顺序逻辑ExecStartPre 会在主进程 ExecStart 之前同步执行,逻辑上满足“在剪贴板服务启动前禁用 THP”的需求。

2. 代码质量

  • 硬编码路径/usr/libexec/dde-thp-disable 使用了硬编码的绝对路径。考虑到项目其他位置使用了 CMake 变量(如 @CMAKE_INSTALL_FULL_BINDIR@),建议保持一致性。
    • 改进建议:如果 dde-thp-disable 也是由该项目构建并安装的,建议在 CMake 配置中将 /usr/libexec 替换为对应的 CMake 变量(例如 @CMAKE_INSTALL_FULL_LIBEXECDIR@),以提高跨平台和不同发行版的兼容性。
  • 代码重复:同样的修改在两个服务文件(dde-clipboard-daemon.service.indde-clipboard.service.in)中重复出现。如果未来需要修改,容易遗漏。
    • 改进建议:如果这两个服务高度相关,可以考虑在 CMake 层面通过 configure_file 共享同一段 Service 配置模板,或者确保有相应的文档/注释说明这两个文件需同步修改。

3. 代码性能

  • 同步阻塞启动ExecStartPre 是同步执行的。如果 /usr/libexec/dde-thp-disable 脚本内部涉及耗时操作(例如等待某个锁、或者执行复杂的磁盘I/O),将会延迟主服务的启动时间。
    • 改进建议:请确认该脚本的执行效率。对于桌面环境的核心组件(剪贴板),启动速度至关重要。该脚本应当是轻量级的,最好是瞬间完成的。如果只是写入如 /sys/kernel/mm/transparent_hugepage/enabled 这样的虚拟文件系统,性能不会有问题。
  • 重复执行开销:如果 dde-clipboard-daemondde-clipboard 有可能同时启动,那么 dde-thp-disable 会被执行两次。
    • 改进建议:由于禁用 THP 通常是系统级别的操作,多次执行虽然不会出错,但属于冗余开销。如果这两个服务总是在同一个会话中启动,可以考虑将 THP 禁用的逻辑移至更上层的、仅启动一次的服务中(例如 session 初始化服务)。

4. 代码安全

  • 权限与特权:修改透明大页(THP)通常需要 root 权限(例如向 /sys/kernel/mm/... 写入数据)。而当前 Service 的 Slice=session.slice 表明这是一个会话级别的服务,通常以普通用户身份运行。
    • 改进建议:请确认 /usr/libexec/dde-thp-disable 是否需要 root 权限。如果需要,该脚本必须配置了相应的权限提升机制(如 sudoers 配置、文件的 SUID 位、或者通过 systemd 的 systemd-sysusers/tmpfiles 赋予了特定Capability),否则该脚本会因为 Permission denied 静默失败(由于加了 - 前缀,主服务会继续启动,但 THP 实际上并未被禁用)。
  • 脚本安全性:确保 /usr/libexec/dde-thp-disable 脚本本身的安全,防止被恶意替换或符号链接攻击。因为即使以普通用户运行,如果脚本具有 SUID 权限,被篡改将导致本地提权漏洞。建议该脚本的文件权限应严格限制为 root:root 755
  • TOCTOU 竞态条件:在服务启动时禁用 THP,如果系统中存在其他服务或定时任务在启用 THP,可能会产生竞态条件。不过对于 THP 这种系统级参数,通常设置后不会频繁变动,此风险较低。

综合改进建议示例

如果你确认需要使用 CMake 变量来替代硬编码路径,修改后的 .service.in 文件应如下所示:

[Service]
Type=dbus
BusName=org.deepin.dde.daemon.Clipboard1
# 使用 CMake 变量替换硬编码路径,确保安装路径的一致性和可配置性
ExecStartPre=-@CMAKE_INSTALL_FULL_LIBEXECDIR@/dde-thp-disable
ExecStart=@CMAKE_INSTALL_FULL_BINDIR@/dde-clipboard-daemon
Slice=session.slice
Restart=on-failure

总结:本次修改逻辑清晰,语法正确,- 前缀的使用避免了预处理失败导致主服务无法启动的问题。重点需要关注的是脚本执行权限问题(普通用户服务能否成功修改内核THP参数)以及硬编码路径的规范性

@mhduiy
Copy link
Copy Markdown
Contributor Author

mhduiy commented May 25, 2026

@mhduiy mhduiy changed the title feat(service): disable THP before clipboard service startup [暂不合并]feat(service): disable THP before clipboard service startup 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