os_wait_semaphore()

os_wait_semaphore()

[Yury Umanets]

Hello all,

I have took a look to acpi_os_wait_semphore() function and realized, 
that the the following code:

                /*
                 * Wait w/ Timeout:
                 * ----------------
                 */
                default:
                // TODO: A better timeout algorithm?
                {
                        int i = 0;
                        static const int quantum_ms = 1000/HZ;
                                                                                        

                        ret = down_trylock(sem);
                        for (i = timeout; (i > 0 && ret < 0); i -= 
quantum_ms) {
                                current->state = TASK_INTERRUPTIBLE;
                                schedule_timeout(1);
                                ret = down_trylock(sem);
                        }
                                                                                        

                        if (ret != 0)
                                status = AE_TIME;
                }
                break;


will behave not very good with Andrea Arcangeli's 2.4.23pre6aa1, where 
some intersting dynamic functionality was introduced. And namely, you 
are able to pass "desktop" or for instnace "HZ=500" as parameter to the 
kernel in lilo or another boot loader. This makes all time slices 
shorter and forces kernel to behave optimally for a desktop machine.

Thus, @quantum_ms will be calculated longer for shorter HZ and this is 
definitelly not good in my opinion. Am I right?

-- 
umka




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

down_timeout

[Matthew Wilcox]

[l-k people, skip to the bottom, that's where down_timeout is]

On Fri, Oct 03, 2003 at 04:37:53PM +0400, Yury Umanets wrote:
> Thus, @quantum_ms will be calculated longer for shorter HZ and this is 
> definitelly not good in my opinion. Am I right?

You're right, but for the wrong reason.  This code is pretty inaccurate
as it's relying on the result of integer divides.  This code should
work better (disclaimer: compiled, not tested):

Index: drivers/acpi/osl.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/acpi/osl.c,v
retrieving revision 1.3
diff -u -p -r1.3 osl.c
--- drivers/acpi/osl.c	23 Aug 2003 02:46:37 -0000	1.3
+++ drivers/acpi/osl.c	3 Oct 2003 14:02:44 -0000
@@ -827,7 +827,6 @@ acpi_os_wait_semaphore(
 {
 	acpi_status		status = AE_OK;
 	struct semaphore	*sem = (struct semaphore*)handle;
-	int			ret = 0;
 
 	ACPI_FUNCTION_TRACE ("os_wait_semaphore");
 
@@ -842,56 +841,28 @@ acpi_os_wait_semaphore(
 	if (in_atomic())
 		timeout = 0;
 
-	switch (timeout)
-	{
-		/*
-		 * No Wait:
-		 * --------
-		 * A zero timeout value indicates that we shouldn't wait - just
-		 * acquire the semaphore if available otherwise return AE_TIME
-		 * (a.k.a. 'would block').
-		 */
-		case 0:
-		if(down_trylock(sem))
-			status = AE_TIME;
-		break;
-
-		/*
-		 * Wait Indefinitely:
-		 * ------------------
-		 */
-		case ACPI_WAIT_FOREVER:
+	if (timeout == ACPI_WAIT_FOREVER) {
 		down(sem);
-		break;
-
-		/*
-		 * Wait w/ Timeout:
-		 * ----------------
-		 */
-		default:
-		// TODO: A better timeout algorithm?
-		{
-			int i = 0;
-			static const int quantum_ms = 1000/HZ;
-
+	} else if (down_trylock(sem) == 0) {
+		/* Success, do nothing */
+	} else {
+		long now = jiffies;
+		int ret = 1;
+		while (jiffies < now + timeout * HZ) {
+			current->state = TASK_INTERRUPTIBLE;
+			schedule_timeout(1);
 			ret = down_trylock(sem);
-			for (i = timeout; (i > 0 && ret < 0); i -= quantum_ms) {
-				current->state = TASK_INTERRUPTIBLE;
-				schedule_timeout(1);
-				ret = down_trylock(sem);
-			}
-	
-			if (ret != 0)
-				status = AE_TIME;
+			if (!ret)
+				break;
 		}
-		break;
+		if (ret)
+			status = AE_TIME;
 	}
 
 	if (ACPI_FAILURE(status)) {
 		ACPI_DEBUG_PRINT ((ACPI_DB_ERROR, "Failed to acquire semaphore[%p|%d|%d], %s\n", 
 			handle, units, timeout, acpi_format_exception(status)));
-	}
-	else {
+	} else {
 		ACPI_DEBUG_PRINT ((ACPI_DB_MUTEX, "Acquired semaphore[%p|%d|%d]\n", handle, units, timeout));
 	}
 

[l-k people, this is the interesting bit]

It's still not great because it doesn't preserve ordering.  down_timeout()
would be a much better primitive.  We have down_interruptible() which
could be used for this purpose.  Something like (completely uncompiled):

/* Returns -EINTR if the timeout expires */
int down_timeout(struct semaphore *sem, long timeout)
{
	struct timer_list timer;
	int result;

	init_timer(&timer);
	timer.expires = timeout + jiffies;
	timer.data = (unsigned long) current;
	timer.function = process_timeout;

	add_timer(&timer);
	result = down_interruptible(sem);
	del_timer_sync(&timer);

	return result;
}

(This would have to go in kernel/timer.c as that's where process_timeout
lives).

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: down_timeout

[Grover, Andrew]

> From: linux-kernel-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 
> [mailto:linux-kernel-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of 
> Matthew Wilcox
> It's still not great because it doesn't preserve ordering.  
> down_timeout()
> would be a much better primitive.  We have down_interruptible() which
> could be used for this purpose.  Something like (completely 
> uncompiled):

Yeah we proposed this 2 years ago and someone (don't remember who) shot
us down.

Regards -- Andy

RE: down_timeout

[Moore, Robert]

I would say that the whole thing is wrong -- the kernel should provide a
semaphore wait function that includes a timeout parameter.

Bob


-----Original Message-----
From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Matthew
Wilcox
Sent: Friday, October 03, 2003 7:25 AM
To: Yury Umanets
Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; linux-kernel@vger.kernel.org
Subject: [ACPI] down_timeout


[l-k people, skip to the bottom, that's where down_timeout is]

On Fri, Oct 03, 2003 at 04:37:53PM +0400, Yury Umanets wrote:
> Thus, @quantum_ms will be calculated longer for shorter HZ and this is

> definitelly not good in my opinion. Am I right?

You're right, but for the wrong reason.  This code is pretty inaccurate
as it's relying on the result of integer divides.  This code should
work better (disclaimer: compiled, not tested):

Index: drivers/acpi/osl.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/acpi/osl.c,v
retrieving revision 1.3
diff -u -p -r1.3 osl.c
--- drivers/acpi/osl.c	23 Aug 2003 02:46:37 -0000	1.3
+++ drivers/acpi/osl.c	3 Oct 2003 14:02:44 -0000
@@ -827,7 +827,6 @@ acpi_os_wait_semaphore(
 {
 	acpi_status		status = AE_OK;
 	struct semaphore	*sem = (struct semaphore*)handle;
-	int			ret = 0;
 
 	ACPI_FUNCTION_TRACE ("os_wait_semaphore");
 
@@ -842,56 +841,28 @@ acpi_os_wait_semaphore(
 	if (in_atomic())
 		timeout = 0;
 
-	switch (timeout)
-	{
-		/*
-		 * No Wait:
-		 * --------
-		 * A zero timeout value indicates that we shouldn't wait
- just
-		 * acquire the semaphore if available otherwise return
AE_TIME
-		 * (a.k.a. 'would block').
-		 */
-		case 0:
-		if(down_trylock(sem))
-			status = AE_TIME;
-		break;
-
-		/*
-		 * Wait Indefinitely:
-		 * ------------------
-		 */
-		case ACPI_WAIT_FOREVER:
+	if (timeout == ACPI_WAIT_FOREVER) {
 		down(sem);
-		break;
-
-		/*
-		 * Wait w/ Timeout:
-		 * ----------------
-		 */
-		default:
-		// TODO: A better timeout algorithm?
-		{
-			int i = 0;
-			static const int quantum_ms = 1000/HZ;
-
+	} else if (down_trylock(sem) == 0) {
+		/* Success, do nothing */
+	} else {
+		long now = jiffies;
+		int ret = 1;
+		while (jiffies < now + timeout * HZ) {
+			current->state = TASK_INTERRUPTIBLE;
+			schedule_timeout(1);
 			ret = down_trylock(sem);
-			for (i = timeout; (i > 0 && ret < 0); i -=
quantum_ms) {
-				current->state = TASK_INTERRUPTIBLE;
-				schedule_timeout(1);
-				ret = down_trylock(sem);
-			}
-	
-			if (ret != 0)
-				status = AE_TIME;
+			if (!ret)
+				break;
 		}
-		break;
+		if (ret)
+			status = AE_TIME;
 	}
 
 	if (ACPI_FAILURE(status)) {
 		ACPI_DEBUG_PRINT ((ACPI_DB_ERROR, "Failed to acquire
semaphore[%p|%d|%d], %s\n", 
 			handle, units, timeout,
acpi_format_exception(status)));
-	}
-	else {
+	} else {
 		ACPI_DEBUG_PRINT ((ACPI_DB_MUTEX, "Acquired
semaphore[%p|%d|%d]\n", handle, units, timeout));
 	}
 

[l-k people, this is the interesting bit]

It's still not great because it doesn't preserve ordering.
down_timeout()
would be a much better primitive.  We have down_interruptible() which
could be used for this purpose.  Something like (completely uncompiled):

/* Returns -EINTR if the timeout expires */
int down_timeout(struct semaphore *sem, long timeout)
{
	struct timer_list timer;
	int result;

	init_timer(&timer);
	timer.expires = timeout + jiffies;
	timer.data = (unsigned long) current;
	timer.function = process_timeout;

	add_timer(&timer);
	result = down_interruptible(sem);
	del_timer_sync(&timer);

	return result;
}

(This would have to go in kernel/timer.c as that's where process_timeout
lives).

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead
bodies.
Do you think I want to have an academic debate on this subject?" --
Robert Fisk


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Acpi-devel mailing list
Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/acpi-devel


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: down_timeout

[Andrew Morton]

Matthew Wilcox <willy-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org> wrote:
>
> /* Returns -EINTR if the timeout expires */
> int down_timeout(struct semaphore *sem, long timeout)
> {
> 	struct timer_list timer;
> 	int result;
> 
> 	init_timer(&timer);
> 	timer.expires = timeout + jiffies;
> 	timer.data = (unsigned long) current;
> 	timer.function = process_timeout;
> 
> 	add_timer(&timer);
> 	result = down_interruptible(sem);
> 	del_timer_sync(&timer);
> 
> 	return result;
> }

down_interruptible() will only break out if signal_pending(current), so the
wakeup-on-expiry here will not work as desired.

New per-arch primitives would be needed to implement this, I think.



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: down_timeout

[Alan Cox]

On Gwe, 2003-10-03 at 21:29, Moore, Robert wrote:
> I would say that the whole thing is wrong -- the kernel should provide a
> semaphore wait function that includes a timeout parameter.

Thats probably the right thing to fix. A timeout aware version of down()
doesnt actually look too hard.



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf