linux-kernel - Re: [PATCH] ipc: Update semtimedop() to use hrtimer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k0baw59j.ffs@tglx>
Date:   Thu, 28 Apr 2022 00:06:16 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Prakash Sangappa <prakash.sangappa@...cle.com>,
        linux-kernel@...r.kernel.org
Cc:     akpm@...ux-foundation.org, peterz@...radead.org,
        prakash.sangappa@...cle.com
Subject: Re: [PATCH] ipc: Update semtimedop() to use hrtimer

Prakash,

On Mon, Apr 18 2022 at 18:51, Prakash Sangappa wrote:
> @@ -1995,7 +1995,10 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>  	int max, locknum;
>  	bool undos = false, alter = false, dupsop = false;
>  	struct sem_queue queue;
> -	unsigned long dup = 0, jiffies_left = 0;
> +	unsigned long dup = 0;
> +	ktime_t expires;
> +	int timed_out = 0;

bool perhaps?

> +	struct timespec64 end_time;
>  
>  	if (nsops < 1 || semid < 0)
>  		return -EINVAL;
> @@ -2008,7 +2011,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,

While at it, can you please replace the open coded validation of timeout
with timespec64_valid()?

>  			error = -EINVAL;
>  			goto out;
>  		}
> -		jiffies_left = timespec64_to_jiffies(timeout);
> +		ktime_get_ts64(&end_time);
> +		end_time = timespec64_add_safe(end_time, *timeout);
> +		expires = timespec64_to_ktime(end_time);

Converting to ktime first makes this cheaper:

                expires = ktime_get() + timespec64_to_ns(timeout);

Less code lines and shorter execution time because adding scalars is
obviously cheaper than adding timespecs.

Now if you add:

       ktime_t expires, *exp = NULL;

then you can do here:

                exp = &expires;
>  	}
>  
>  
> @@ -2167,7 +2172,9 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>  		rcu_read_unlock();
>  
>  		if (timeout)
> -			jiffies_left = schedule_timeout(jiffies_left);
> +			timed_out = !schedule_hrtimeout_range(&expires,
> +						current->timer_slack_ns,
> +						HRTIMER_MODE_ABS);
>  		else
>  			schedule();

and this can be simplified to:

                timed_out = !schedule_hrtimeout_range(exp, current->timer_slack_ns,
						      HRTIMER_MODE_ABS)

schedule_hrtimeout_range() directly invokes schedule() when @exp == NULL
and returns != 0 when woken up in that case.

> @@ -2210,7 +2217,7 @@ long __do_semtimedop(int semid, struct sembuf *sops,
>  		/*
>  		 * If an interrupt occurred we have to clean up the queue.
>  		 */
> -		if (timeout && jiffies_left == 0)
> +		if (timeout && timed_out)

and this becomes

                if (timed_out)

>  			error = -EAGAIN;
>  	} while (error == -EINTR && !signal_pending(current)); /* spurious */

Hmm?

Done right, you should end up with a negative diffstat :)

Thanks,

        tglx