Re: [PATCH 4/4] block: update cached timestamp post schedule/preemption

[Jens Axboe <axboe@kernel.dk>]

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