[PATCH] Re: mq_timedrecieve timeout accuracy

[Carsten Emde <Carsten.Emde@osadl.org>]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

Hi Prady,

> [..] Ok, as promised.
> 
> Here is a dirty hack that I made which basically resulted in 2 things.
> 1) My test programs accurace really really improved. The timeout on
> the mq_timedrecieve this time around within the tune of 20-30uS.
> 2) My actual application exploded all over the place, with mq_*
> functions complaining of timing out etc etc .. Just a bad messup.
> 
> So (1) confirms that sched_hrtimeout could be a good idea. (2)
> Confirms that this patch is really terrible and it would be great if
> someone could either come up with something that is more robust or I
> will be happy to take suggestions on where and how I should make
> changes.
Does the attached patch work for you?

- Fixed error handling
- Replaced schedule_timeout() with schedule_hrtimeout()
- Let schedule_hrtimeout() handle expired timers

Signed-off-by: Carsten Emde <C.Emde@osadl.org>

[-- Attachment #2: fix-mqueue-and-use-hrtimer.patch --]
[-- Type: text/x-patch, Size: 3606 bytes --]

--- ipc/mqueue-orig.c	2010-03-29 13:59:14.410923036 +0200
+++ ipc/mqueue.c	2010-03-29 16:38:51.650616863 +0200
@@ -428,10 +428,16 @@
  * sr: SEND or RECV
  */
 static int wq_sleep(struct mqueue_inode_info *info, int sr,
-			long timeout, struct ext_wait_queue *ewp)
+			struct timespec *timeout, struct ext_wait_queue *ewp)
 {
 	int retval;
 	signed long time;
+	ktime_t *expires = NULL, to;
+
+	if (timeout != NULL) {
+		to = timespec_to_ktime(*timeout);
+		expires = &to;
+	}
 
 	wq_add(info, sr, ewp);
 
@@ -439,7 +445,7 @@
 		set_current_state(TASK_INTERRUPTIBLE);
 
 		spin_unlock(&info->lock);
-		time = schedule_timeout(timeout);
+		time = schedule_hrtimeout(expires, HRTIMER_MODE_ABS);
 
 		while (ewp->state == STATE_PENDING)
 			cpu_relax();
@@ -551,33 +557,6 @@
 	wake_up(&info->wait_q);
 }
 
-static long prepare_timeout(struct timespec *p)
-{
-	struct timespec nowts;
-	long timeout;
-
-	if (p) {
-		if (unlikely(p->tv_nsec < 0 || p->tv_sec < 0
-			|| p->tv_nsec >= NSEC_PER_SEC))
-			return -EINVAL;
-		nowts = CURRENT_TIME;
-		/* first subtract as jiffies can't be too big */
-		p->tv_sec -= nowts.tv_sec;
-		if (p->tv_nsec < nowts.tv_nsec) {
-			p->tv_nsec += NSEC_PER_SEC;
-			p->tv_sec--;
-		}
-		p->tv_nsec -= nowts.tv_nsec;
-		if (p->tv_sec < 0)
-			return 0;
-
-		timeout = timespec_to_jiffies(p) + 1;
-	} else
-		return MAX_SCHEDULE_TIMEOUT;
-
-	return timeout;
-}
-
 static void remove_notification(struct mqueue_inode_info *info)
 {
 	if (info->notify_owner != NULL &&
@@ -861,7 +840,6 @@
 	struct msg_msg *msg_ptr;
 	struct mqueue_inode_info *info;
 	struct timespec ts, *p = NULL;
-	long timeout;
 	int ret;
 
 	if (u_abs_timeout) {
@@ -875,7 +853,6 @@
 		return -EINVAL;
 
 	audit_mq_sendrecv(mqdes, msg_len, msg_prio, p);
-	timeout = prepare_timeout(p);
 
 	ret = -EBADF;
 	filp = fget(mqdes);
@@ -912,14 +889,17 @@
 		if (filp->f_flags & O_NONBLOCK) {
 			spin_unlock(&info->lock);
 			ret = -EAGAIN;
-		} else if (unlikely(timeout < 0)) {
-			spin_unlock(&info->lock);
-			ret = timeout;
 		} else {
-			wait.task = current;
-			wait.msg = (void *) msg_ptr;
-			wait.state = STATE_NONE;
-			ret = wq_sleep(info, SEND, timeout, &wait);
+			if (p && unlikely(ts.tv_nsec < 0 || ts.tv_sec < 0 ||
+			    ts.tv_nsec >= NSEC_PER_SEC)) {
+				spin_unlock(&info->lock);
+                	        ret = -EINVAL;
+			} else {
+				wait.task = current;
+				wait.msg = (void *) msg_ptr;
+				wait.state = STATE_NONE;
+				ret = wq_sleep(info, SEND, p, &wait);
+			}
 		}
 		if (ret < 0)
 			free_msg(msg_ptr);
@@ -947,7 +927,6 @@
 		size_t, msg_len, unsigned int __user *, u_msg_prio,
 		const struct timespec __user *, u_abs_timeout)
 {
-	long timeout;
 	ssize_t ret;
 	struct msg_msg *msg_ptr;
 	struct file *filp;
@@ -964,7 +943,6 @@
 	}
 
 	audit_mq_sendrecv(mqdes, msg_len, 0, p);
-	timeout = prepare_timeout(p);
 
 	ret = -EBADF;
 	filp = fget(mqdes);
@@ -991,16 +969,17 @@
 		if (filp->f_flags & O_NONBLOCK) {
 			spin_unlock(&info->lock);
 			ret = -EAGAIN;
-			msg_ptr = NULL;
-		} else if (unlikely(timeout < 0)) {
-			spin_unlock(&info->lock);
-			ret = timeout;
-			msg_ptr = NULL;
 		} else {
-			wait.task = current;
-			wait.state = STATE_NONE;
-			ret = wq_sleep(info, RECV, timeout, &wait);
-			msg_ptr = wait.msg;
+			if (p && unlikely(ts.tv_nsec < 0 || ts.tv_sec < 0 ||
+			    ts.tv_nsec >= NSEC_PER_SEC)) {
+				spin_unlock(&info->lock);
+                	        ret = -EINVAL;
+			} else {
+				wait.task = current;
+				wait.state = STATE_NONE;
+				ret = wq_sleep(info, RECV, p, &wait);
+				msg_ptr = wait.msg;
+			}
 		}
 	} else {
 		msg_ptr = msg_get(info);