Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: don't check RLIMIT_RTPRIO #1083

Merged
merged 3 commits into from
Jun 26, 2023
Merged

core: don't check RLIMIT_RTPRIO #1083

merged 3 commits into from
Jun 26, 2023

Conversation

yshui
Copy link
Owner

@yshui yshui commented Jun 24, 2023

FreeBSD doesn't have RLIMIT_RTPRIO. So instead we skip this check and
just always try to set our priority to the lowest SCHED_RR priority
available.

Fixes #1082

Signed-off-by: Yuxuan Shui yshuiv7@gmail.com

FreeBSD doesn't have RLIMIT_RTPRIO. So instead we skip this check and
just always try to set our priority to the lowest SCHED_RR priority
available.

Fixes #1082

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Don't use pthread_{set,get}schedparam, which requires -lpthread. Use
sched_setscheduler/sched_getparam instead, which is provided by libc.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
@absolutelynothelix
Copy link
Collaborator

the approach taken to mitigate the mentioned issue looks good to me, i like it.

however, i have a few concerns about the scheduling stuff overall:

  1. you’ve added a message suggesting to give picom the CAP_SYS_NICE capability when picom fails to set the priority. iirc, it failed for me from the very beginning but i didn’t know why and have never looked into it. now it looks like that i was missing this capability. does this work at all without it? because if it doesn’t i don’t really like the idea of giving any kind of elevated permissions to a compositor since i can’t imagine a scenario where it actually needs them;
  2. i missed the exact moment when all this scheduling stuff was introduced with frame pacing so it’d be good to add a comment briefly explaining what this functions does and the most importantly why do we need it.

@yshui
Copy link
Owner Author

yshui commented Jun 25, 2023

@absolutelynothelix reason for the scheduling priority is explained in: 7d96923 I could add a comment for that.

no, CAP_SYS_NICE is not mandatory, you can also raise your ulimit -r. picom needs the permission to enable realtime scheduling strategy.

@absolutelynothelix
Copy link
Collaborator

@yshui, ah, i thought that it’s a small part of a big mechanism (frame pacing) but it seems that it’s a standalone useful addition. the commit description makes it clear. i’d use it as a comment describing the set_rr_scheduling function and add a note about CAP_SYS_NICE and ulimit -r.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #1083 (f8cdc81) into next (b852723) will increase coverage by 0.14%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1083      +/-   ##
==========================================
+ Coverage   37.53%   37.67%   +0.14%     
==========================================
  Files          49       49              
  Lines       11169    11168       -1     
==========================================
+ Hits         4192     4208      +16     
+ Misses       6977     6960      -17     
Impacted Files Coverage Δ
src/picom.c 62.09% <83.33%> (+0.63%) ⬆️

... and 2 files with indirect coverage changes

@absolutelynothelix absolutelynothelix merged commit 0bb45d5 into next Jun 26, 2023
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.

Commit 7d96923 breaks non-Linux
2 participants