Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Prakash Sangappa <prakash.sangappa@oracle.com>,
        linux-kernel@vger.kernel.org
Cc:     akpm@linux-foundation.org, peterz@infradead.org,
        prakash.sangappa@oracle.com
Subject: Re: [PATCH] ipc: Update semtimedop() to use hrtimer
In-Reply-To: <1650333099-27214-1-git-send-email-prakash.sangappa@oracle.com>
References: <1650333099-27214-1-git-send-email-prakash.sangappa@oracle.com>
Date:   Thu, 28 Apr 2022 00:06:16 +0200
Message-ID: <87k0baw59j.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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