On 1/29/24 7:06 AM, Johannes Thumshirn wrote:
> On 29.01.24 15:02, Jens Axboe wrote:
>> On 1/29/24 1:01 AM, Johannes Thumshirn wrote:
>>> On 26.01.24 22:39, Jens Axboe wrote:
>>>> static void sched_update_worker(struct task_struct *tsk)
>>>> {
>>>> - if (tsk->flags & (PF_WQ_WORKER | PF_IO_WORKER)) {
>>>> + if (tsk->flags & (PF_WQ_WORKER | PF_IO_WORKER | PF_BLOCK_TS)) {
>>>> + if (tsk->flags & PF_BLOCK_TS)
>>>> + blk_plug_invalidate_ts(tsk);
>>>> if (tsk->flags & PF_WQ_WORKER)
>>>> wq_worker_running(tsk);
>>>> - else
>>>> + else if (tsk->flags & PF_IO_WORKER)
>>>> io_wq_worker_running(tsk);
>>>> }
>>>> }
>>>
>>>
>>> Why the nested if? Isn't that more readable:
>>
>> It's so that we can keep it at a single branch for the fast case of none
>> of them being true, which is also how it was done before this change.
>> This one just adds one more flag to check. With your change, it's 3
>> branches instead of one for the fast case.
>>
>
> Although I don't really have hard feelings for it, that could be solved
> as well:
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9116bcc90346..74beb0126da6 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6787,12 +6787,12 @@ static inline void sched_submit_work(struct
> task_struct *tsk)
>
> static void sched_update_worker(struct task_struct *tsk)
> {
> - if (tsk->flags & (PF_WQ_WORKER | PF_IO_WORKER)) {
> - if (tsk->flags & PF_WQ_WORKER)
> - wq_worker_running(tsk);
> - else
> - io_wq_worker_running(tsk);
> - }
> + if (tsk->flags & !(PF_WQ_WORKER | PF_IO_WORKER | PF_BLOCK_TS))
> + return;
Don't think that'd work :-)
> + if (tsk->flags & PF_BLOCK_TS)
> + blk_plug_invalidate_ts(tsk);
> + if (tsk->flags & PF_WQ_WORKER)
> + wq_worker_running(tsk);
> + else if (tsk->flags & PF_IO_WORKER)
> + io_wq_worker_running(tsk);
> }
>
> But yep, that's bikeshedding I admit
But agree, it'd accomplish the same. The patch is cleaner just keeping
the existing setup, however, rather than rewrite it like the above.
--
Jens Axboe
