Re: [PATCH v3 1/1] hrtimer: use and report correct timerslack values for realtime tasks

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Aug 13 2024 at 09:29, Felix Moessbauer wrote:
> @@ -2569,10 +2569,11 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>  	}
>  
>  	task_lock(p);
> -	if (slack_ns == 0)
> -		p->timer_slack_ns = p->default_timer_slack_ns;
> -	else
> -		p->timer_slack_ns = slack_ns;
> +	if (task_is_realtime(p))
> +		slack_ns = 0;

This should respect the user supplied value, i.e.

    if (!task_is_realtime(p) && !slack_ns)
	    slack_ns = p->default_timer_slack_ns;

> +	else if (slack_ns == 0)
> +		slack_ns = p->default_timer_slack_ns;
> +	p->timer_slack_ns = slack_ns;
>  	task_unlock(p);
>  
>  out:
> diff --git a/fs/select.c b/fs/select.c
> index 9515c3fa1a03..153124ed50fd 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -77,19 +77,13 @@ u64 select_estimate_accuracy(struct timespec64 *tv)
>  {
>  	u64 ret;
>  	struct timespec64 now;
> -
> -	/*
> -	 * Realtime tasks get a slack of 0 for obvious reasons.
> -	 */
> -
> -	if (rt_task(current))
> -		return 0;
> +	u64 slack = current->timer_slack_ns;
>  
>  	ktime_get_ts64(&now);
>  	now = timespec64_sub(*tv, now);
>  	ret = __estimate_accuracy(&now);
> -	if (ret < current->timer_slack_ns)
> -		return current->timer_slack_ns;
> +	if (ret < slack || slack == 0)
> +		return slack;

Seriously? Do all the calculations first and then discard them when
slack is 0?

> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> index ae1b42775ef9..195d2f2834a9 100644
> --- a/kernel/sched/syscalls.c
> +++ b/kernel/sched/syscalls.c
> @@ -406,6 +406,14 @@ static void __setscheduler_params(struct task_struct *p,
>  	else if (fair_policy(policy))
>  		p->static_prio = NICE_TO_PRIO(attr->sched_nice);
>  
> +	/* rt-policy tasks do not have a timerslack */
> +	if (task_is_realtime(p)) {
> +		p->timer_slack_ns = 0;
> +	} else if (p->timer_slack_ns == 0) {
> +		/* when switching back to non-rt policy, restore timerslack */
> +		p->timer_slack_ns = p->default_timer_slack_ns;
> +	}
> +
>  	/*
>  	 * __sched_setscheduler() ensures attr->sched_priority == 0 when
>  	 * !rt_policy. Always setting this ensures that things like
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 3a2df1bd9f64..e3c4cffb520c 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2557,6 +2557,8 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  			error = current->timer_slack_ns;
>  		break;
>  	case PR_SET_TIMERSLACK:
> +		if (task_is_realtime(current))
> +			break;

Why are you declaring that a RT task has to have 0 slack if we are
lifting the hard coded slack zeroing in the hrtimer functions?

Thanks,

        tglx