| Message ID | 20250219182020.393006-1-jemmywong512@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | sched: Refine scheduler naming for clarity and specificity | expand |
On Thu, 20 Feb 2025 02:20:18 +0800 Jemmy Wong <jemmywong512@gmail.com> wrote: > Hi, I'm relatively new to Linux and eager to contribute to the community > with some foundational work. Welcome to the community. > > I aim to help improve code readability and maintainability. > While reading the scheduler code, I found some naming inconsistencies > that made understanding the code more difficult. I do plan on updating the scheduler comments soon, so that every function has a purpose. But there's a lot of history to the scheduler code that you really can't just go and rename items. It causes a lot of churn which also causes noise in the git history, where git blame is used a lot to find why things were done. Adding renames causes one more level of indirection that makes it harder on the maintainers to do git forensics. > > Specifically: > 1. Some function names do not accurately reflect their purpose. > 2. Certain names are too general, making it unclear what they represent. > 3. Some concepts are ambiguous, leading to potential confusion. > > - Rename ttwu_do_wakeup to ttwu_set_running: > - This function only sets task state to TASK_RUNNING, > not performing the actual wakeup. > > - Rename update_cfs_group to update_cfs_group_shares: > - The name was too generic; specifying "shares" clarifies its purpose. > > - Rename place_entity to update_entity_sched: > - The function does not handle entity placement but updates > sched info: vruntime, slice, and deadline. > > - Rename update_load_<set, add, sub> to load_weight_<set, add, sub>: > - "load" can refer to either PELT load or load weight, causing ambiguity; > "load_weight" specifies it's dealing with weight. > > - Rename struct sched_avg to struct sched_pelt: > - This structure includes not just average statistics > but also sums like <load, runnable, util>_sum, last_updae_time, > all of which are PELT (Per-Entity Load Tracking) metrics. > > - Rename init_entity_runnable_average to init_entity_pelt > > This patch aims to improve code readability and reduce confusion by > ensuring names are more descriptive of their actual functionality or purpose. I personally am not against these updates. But as I mentioned, there's a lot of history here and it's really Peter Zijlstra's call (and he's been against changes like this in the past). So please do not be discourage if this doesn't get much traction. -- Steve
Thanks for filling me in on the background of the code status and the potential consequences of these changes. I really appreciate the context! As Phil Karlton once said: There are only two hard things in Computer Science: cache invalidation and naming things. A good name makes code easier to read and understand. I personally value code readability and maintainability. However, based on your input and the community’s opinion, I agree that these changes could introduce indirection for git blame and make maintenance even harder. So, I’ll discard the changes. BR, Jemmy > On Feb 20, 2025, at 6:24 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 20 Feb 2025 02:20:18 +0800 > Jemmy Wong <jemmywong512@gmail.com> wrote: > >> Hi, I'm relatively new to Linux and eager to contribute to the community >> with some foundational work. > > Welcome to the community. > >> >> I aim to help improve code readability and maintainability. >> While reading the scheduler code, I found some naming inconsistencies >> that made understanding the code more difficult. > > I do plan on updating the scheduler comments soon, so that every function > has a purpose. But there's a lot of history to the scheduler code that you > really can't just go and rename items. It causes a lot of churn which also > causes noise in the git history, where git blame is used a lot to find why > things were done. Adding renames causes one more level of indirection that > makes it harder on the maintainers to do git forensics. > >> >> Specifically: >> 1. Some function names do not accurately reflect their purpose. >> 2. Certain names are too general, making it unclear what they represent. >> 3. Some concepts are ambiguous, leading to potential confusion. >> >> - Rename ttwu_do_wakeup to ttwu_set_running: >> - This function only sets task state to TASK_RUNNING, >> not performing the actual wakeup. >> >> - Rename update_cfs_group to update_cfs_group_shares: >> - The name was too generic; specifying "shares" clarifies its purpose. >> >> - Rename place_entity to update_entity_sched: >> - The function does not handle entity placement but updates >> sched info: vruntime, slice, and deadline. >> >> - Rename update_load_<set, add, sub> to load_weight_<set, add, sub>: >> - "load" can refer to either PELT load or load weight, causing ambiguity; >> "load_weight" specifies it's dealing with weight. >> >> - Rename struct sched_avg to struct sched_pelt: >> - This structure includes not just average statistics >> but also sums like <load, runnable, util>_sum, last_updae_time, >> all of which are PELT (Per-Entity Load Tracking) metrics. >> >> - Rename init_entity_runnable_average to init_entity_pelt >> >> This patch aims to improve code readability and reduce confusion by >> ensuring names are more descriptive of their actual functionality or purpose. > > I personally am not against these updates. But as I mentioned, there's a > lot of history here and it's really Peter Zijlstra's call (and he's been > against changes like this in the past). > > So please do not be discourage if this doesn't get much traction. > > -- Steve
Hi, I'm relatively new to Linux and eager to contribute to the community with some foundational work. I aim to help improve code readability and maintainability. While reading the scheduler code, I found some naming inconsistencies that made understanding the code more difficult. Specifically: 1. Some function names do not accurately reflect their purpose. 2. Certain names are too general, making it unclear what they represent. 3. Some concepts are ambiguous, leading to potential confusion. - Rename ttwu_do_wakeup to ttwu_set_running: - This function only sets task state to TASK_RUNNING, not performing the actual wakeup. - Rename update_cfs_group to update_cfs_group_shares: - The name was too generic; specifying "shares" clarifies its purpose. - Rename place_entity to update_entity_sched: - The function does not handle entity placement but updates sched info: vruntime, slice, and deadline. - Rename update_load_<set, add, sub> to load_weight_<set, add, sub>: - "load" can refer to either PELT load or load weight, causing ambiguity; "load_weight" specifies it's dealing with weight. - Rename struct sched_avg to struct sched_pelt: - This structure includes not just average statistics but also sums like <load, runnable, util>_sum, last_updae_time, all of which are PELT (Per-Entity Load Tracking) metrics. - Rename init_entity_runnable_average to init_entity_pelt This patch aims to improve code readability and reduce confusion by ensuring names are more descriptive of their actual functionality or purpose. Signed-off-by: Jemmy Wong <Jemmywong512@gmail.com> --- Jemmy Wong (2): sched: Refine scheduler naming for clarity and specificity sched: Refine sched_avg naming for clarity and specificity Documentation/trace/ftrace.rst | 190 ++++++------- include/linux/sched.h | 6 +- kernel/sched/core.c | 18 +- kernel/sched/debug.c | 36 +-- kernel/sched/fair.c | 478 ++++++++++++++++----------------- kernel/sched/pelt.c | 100 +++---- kernel/sched/pelt.h | 16 +- kernel/sched/rt.c | 2 +- kernel/sched/sched.h | 22 +- 9 files changed, 433 insertions(+), 435 deletions(-) -- 2.43.0