[PATCH] Syslets - Fix cachemiss_thread return value

[PATCH] Syslets - Fix cachemiss_thread return value

[Jeff Dike]

cachemiss_thread should explicitly return 0 or error instead of
task_ret_reg(current) (which is -ENOSYS anyway) because
async_thread_helper is careful to put the return value in eax anyway.

On x86_64, it looks like async_child_rip is similarly careful.

Signed-off-by: Jeff Dike <jdike@linux.intel.com>
--
 kernel/async.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/kernel/async.c
===================================================================
--- linux-2.6.orig/kernel/async.c	2007-05-29 20:11:11.000000000 -0400
+++ linux-2.6/kernel/async.c	2007-05-31 14:12:49.000000000 -0400
@@ -575,7 +575,7 @@ static long cachemiss_thread(void *data)
 	struct task_struct *t = current;
 	struct async_head *ah = args->ah;
 	struct async_thread *at;
-	int ret;
+	int ret = 0;
 
 	at = &t->__at;
 	async_thread_init(t, at, ah);
@@ -607,7 +607,7 @@ static long cachemiss_thread(void *data)
 	complete(&ah->start_done);
 
 	async_cachemiss_loop(at, ah, t);
-	return task_ret_reg(t);
+	return ret;
 }
 
 /**

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Ingo Molnar]

* Jeff Dike <jdike@addtoit.com> wrote:

> cachemiss_thread should explicitly return 0 or error instead of 
> task_ret_reg(current) (which is -ENOSYS anyway) because 
> async_thread_helper is careful to put the return value in eax anyway.

oops, indeed! Thanks.

	Ingo

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Zach Brown]

> cachemiss_thread should explicitly return 0 or error instead of
> task_ret_reg(current) (which is -ENOSYS anyway) because
> async_thread_helper is careful to put the return value in eax anyway.

Can you explain what motivated you to send out this patch?

It used to return 0.  It was changed because, unlike the syslet
syscalls, sys_io_submit() doesn't have a simple 0 value to indicate
success.  The current implementation wants to return the number of iocbs
that were processed, including the one which blocked, from the cachemiss
thread.  So before calling into an operation it sets task_ret_ret() so
that the cachemiss thread can return it if it takes over.
task_ret_reg() is holding a return value that is being returned by the
cachemiss thread on behalf of a sys_io_submit() which blocked.

When I made the change I didn't really audit its effect on the other
paths.  I suppose it's time to do that, and you could help by telling me
if you saw something bad happen :).

- z

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Jeff Dike]

On Thu, May 31, 2007 at 03:07:16PM -0700, Zach Brown wrote:
> Can you explain what motivated you to send out this patch?
> 
> It used to return 0.  It was changed because, unlike the syslet
> syscalls, sys_io_submit() doesn't have a simple 0 value to indicate
> success.  The current implementation wants to return the number of iocbs
> that were processed, including the one which blocked, from the cachemiss
> thread.  So before calling into an operation it sets task_ret_ret() so
> that the cachemiss thread can return it if it takes over.
> task_ret_reg() is holding a return value that is being returned by the
> cachemiss thread on behalf of a sys_io_submit() which blocked.
> 
> When I made the change I didn't really audit its effect on the other
> paths.  I suppose it's time to do that, and you could help by telling me
> if you saw something bad happen :).

The bad thing was sys_async_exec returning -NOSYS every time a new
cachemiss thread was created.

Without this patch, you'll see sys_async_exec failures with either of
the demos I sent out.  Dunno about the existing ones, but I bet they do
the same.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Zach Brown]

> the demos I sent out.  Dunno about the existing ones, but I bet they do
> the same.

Hmm, they didn't when I ran them, but I'll give yours a try and take a
closer look.  Thanks for taking the time to bring it up.

- z

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Zach Brown]

On Thu, May 31, 2007 at 02:19:23PM -0400, Jeff Dike wrote:
> cachemiss_thread should explicitly return 0 or error instead of
> task_ret_reg(current) (which is -ENOSYS anyway) because
> async_thread_helper is careful to put the return value in eax anyway.

Here's the fix I came up with for this.  Untested (my test boxes are
busy doing some aio+syslet perf runs :)).  What do you guys think?

- z

- - -

async: return task_ret_reg() from async_cachemiss_loop()

When a thread blocks in __async_schedule() it finds a thread waiting in
async_cachemiss_loop() to return to userspace.  The return value of
async_cachemiss_loop() is propagated to userspace as the return code of the
call which the thread blocked in.

Previously this return code was hard-coded to 0 and -1, which worked fine for
the first syslet syscall APIs.  sys_io_submit() wants a different return call
convention when it uses syslets.  So cachemiss_thread() was changed to return
task_ret_reg(), sys_io_submit() would set it to the value it wanted returned to
userspace.

But this, by itself, was buggy.  The syslet syscall APIs didn't set
task_ret_reg() like sys_io_submit() did, so they got garbage returned.  Both
Jeff Dike and Chris Mason saw this in testing, though I was lucky enough to
have my returned garbage pass the tests I initially tried.  It also didn't
catch the other callers of async_cachemiss_loop() which would be returning to
userspace on behalf of calls which blocked in __async_schedule().  This would
be a problem, say, if people mixed sys_threadlet_on() and sys_io_submit() in
the same task.

So this patch fixes those up.

We always return task_ret_reg() from async_cachemiss_loop().  If we're
returning to the user's stack and IP instead of on behalf of a blocking call we
set task_ret_reg() to -1 and then return it.

__exec_atom() sets task_ret_reg() to NULL if there's a chance that it will
block while executing the syscall in the atom.

Signed-off-by: Zach Brown <zach.brown@oracle.com>

diff -r f0d8ee165e2e kernel/async.c
--- a/kernel/async.c	Thu Jun 07 14:32:31 2007 -0700
+++ b/kernel/async.c	Thu Jun 07 16:14:16 2007 -0700
@@ -206,6 +206,13 @@ static long __exec_atom(struct task_stru
 
 	if (unlikely(atom->nr >= atom->nr_syscalls))
 		return -ENOSYS;
+
+	/*
+	 * If the syslet goes asynchronous then we want the cachemiss
+	 * thread to return NULL to userspace on our behalf.
+	 */
+	if (likely(t->async_ready))
+		task_ret_reg(t) = 0;
 
 	ret = atom->call_table[atom->nr](atom->args[0], atom->args[1],
 					 atom->args[2], atom->args[3],
@@ -515,7 +522,22 @@ exec_atom(struct async_head *ah, struct 
 
 	return last_uatom;
 }
-
+/**
+ * async_cachemiss_loop - wait as a "cachemiss" thread
+ * @at:	this thread's async_thread in its task_struct
+ * @ah:	The async_head for this thread's group of threads
+ * @t:	this thread's task_struct
+ *
+ * Sleep as a ready async "cachemiss" thread.  If another thread in our
+ * async_head blocks then __async_schedule may block that thread and transfer
+ * its userspace over to us to return in to.
+ *
+ * Before threads get to __async_schedule() they set task_ret_reg() to the
+ * value that should be returned to userspace if they block.  We return it so
+ * that our caller can return it to userspace, either through the usual syscall
+ * return path or through the helper which cachemiss_thread() returns to after
+ * being created.
+ */
 long async_cachemiss_loop(struct async_thread *at, struct async_head *ah,
 			  struct task_struct *t)
 {
@@ -542,10 +564,13 @@ long async_cachemiss_loop(struct async_t
 		 */
 		set_task_stack_reg(t, at->user_stack);
 		task_ip_reg(t) = at->user_ip;
-
-		return -1;
-	}
-	return 0;
+		/*
+		 * We're not returning on behalf of some call, communicate
+		 * that.
+		 */
+		task_ret_reg(t) = -1;
+	}
+	return task_ret_reg(t);
 }
 
 struct cachemiss_args {
@@ -559,14 +584,6 @@ struct cachemiss_args {
  * first time: initialize, pick up the user stack/IP addresses from
  * the head and then execute the cachemiss loop. If the cachemiss
  * loop returns then we return back to user-space.
- *
- * Our return code overwrites the task_ret_reg() in ptregs in the assembly
- * stub which execution returns to before returning to userspace.  We may
- * be returning to userspace on behalf of an interrupted call which might
- * want us to return something specific to user space.  We return 
- * task_ret_reg() from this function so that the tasks we're returning
- * on behalf of can use it to pass return codes to userspace.  Perhaps
- * we shouldn't have the asm stub overwrite task_ret_reg() instead..
  */
 static long cachemiss_thread(void *data)
 {
@@ -606,8 +623,7 @@ static long cachemiss_thread(void *data)
 	}
 	complete(&ah->start_done);
 
-	async_cachemiss_loop(at, ah, t);
-	return task_ret_reg(t);
+	return async_cachemiss_loop(at, ah, t);
 }
 
 /**

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Jeff Dike]

On Thu, Jun 07, 2007 at 04:27:33PM -0700, Zach Brown wrote:
> On Thu, May 31, 2007 at 02:19:23PM -0400, Jeff Dike wrote:
> > cachemiss_thread should explicitly return 0 or error instead of
> > task_ret_reg(current) (which is -ENOSYS anyway) because
> > async_thread_helper is careful to put the return value in eax anyway.
> 
> Here's the fix I came up with for this.  Untested (my test boxes are
> busy doing some aio+syslet perf runs :)).  What do you guys think?

I don't like it :-)

It's better though.  With the test below (a hacked up version of
aio-read.c), I still get -ENOSYS leaking out of sys_async_exec when it
tries an async setuid.

I haven't started looking into this yet.

				Jeff

-- 
Work email - jdike at linux dot intel dot com


#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <linux/unistd.h>
#include <sys/fcntl.h>
#include <sys/stat.h>
#include "sys.h"

/*
 * Set up a syslet atom:
 */
static void
init_atom(struct syslet_uatom *atom, int nr,
	  void *arg_ptr0, void *arg_ptr1, void *arg_ptr2,
	  void *arg_ptr3, void *arg_ptr4, void *arg_ptr5,
	  void *ret_ptr, unsigned long flags, struct syslet_uatom *next)
{
	atom->nr = nr;
	atom->arg_ptr[0] = (u64)(unsigned long)arg_ptr0;
	atom->arg_ptr[1] = (u64)(unsigned long)arg_ptr1;
	atom->arg_ptr[2] = (u64)(unsigned long)arg_ptr2;
	atom->arg_ptr[3] = (u64)(unsigned long)arg_ptr3;
	atom->arg_ptr[4] = (u64)(unsigned long)arg_ptr4;
	atom->arg_ptr[5] = (u64)(unsigned long)arg_ptr5;
	atom->ret_ptr = (u64)(unsigned long)ret_ptr;
	atom->flags = flags;
	atom->next = (u64)(unsigned long)next;
}

#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))

static int collect_status(void)
{
	int n = 0;

	sys_async_wait(1, async_head.user_ring_idx, &async_head);
	while(completion_ring[async_head.user_ring_idx] != 0){
		completion_ring[async_head.user_ring_idx++] = 0;
		async_head.user_ring_idx %= ARRAY_SIZE(completion_ring);
		n++;
	}

	return n;
}

int oneshot(struct syslet_uatom *atom, int syscall)
{
	struct syslet_uatom *done;
	int err = -1;

	init_atom(atom, __NR_getuid, NULL, NULL, NULL, NULL, NULL, NULL,
		  &err, SYSLET_ASYNC, NULL);
	if(async_head.new_thread_stack == 0)
		async_head.new_thread_stack = thread_stack_alloc();
	done = sys_async_exec(atom, &async_head);
	if(done == atom)
		printf("setuid was synchronous\n");
	else if(done == NULL){
		collect_status();
	}
	else {
		perror("async setuid");
		exit(1);
	}

	return err;
}

int main(int argc, char *argv[])
{
	struct syslet_uatom *atoms, *done;
	struct stat stbuf;
	char *file, *buf;
	int fd, err, npages, i, len, async, sync;

	if(argc < 2){
		fprintf(stderr, "Usage : aio-read file\n");
		exit(1);
	}

	file = argv[1];
	fd = open(file, O_RDONLY);
	if(fd < 0){
		perror("open");
		exit(1);
	}

	err = fstat(fd, &stbuf);
	if(err < 0){
		perror("stat");
		exit(1);
	}

	buf = malloc(stbuf.st_size);
	if(buf == NULL){
		perror("malloc");
		exit(1);
	}

	len = getpagesize();
	npages = (stbuf.st_size + len - 1) / len;
	atoms = malloc(npages * sizeof(struct syslet_uatom));
	if(atoms == NULL){
		perror("malloc atoms");
		exit(1);
	}

	async_head_init();
	async = 0;
	sync = 0;
	printf("head pid = %d, uid = %d\n", getpid(), getuid());
	for(i = 0; i < npages; i++){
		char *ptr = &buf[i * len];
		init_atom(&atoms[i], __NR_sys_read, &fd, &ptr, &len,
			  NULL, NULL, NULL, NULL, 0, NULL);
		if(async_head.new_thread_stack == 0)
			async_head.new_thread_stack = thread_stack_alloc();
		done = sys_async_exec(&atoms[i], &async_head);
		if(done == &atoms[i]){
			sync++;
			continue;
		}
		else if(done < 0)
			perror("sys_async_exec");

		async++;
		printf("async = %d, pid = %ld\n", async, syscall(__NR_gettid));
		if(async < ARRAY_SIZE(completion_ring))
			continue;

		async -= collect_status();
	}

	while(async)
		async -= collect_status();

	if(setuid(500) < 0){
		perror("setuid");
		exit(1);
	}

	err = oneshot(&atoms[0], __NR_getuid);
	printf("uid1 = %d\n", err);
	printf("current uid1 = %d\n", getuid());
	
	err = oneshot(&atoms[0], __NR_getuid);
	printf("uid2 = %d\n", err);
	printf("current uid2 = %d\n", getuid());

	err = oneshot(&atoms[0], __NR_getuid);
	printf("uid3 = %d\n", err);
	printf("current uid3 = %d\n", getuid());

	pause();

	return 0;
}

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Zach Brown]

> I don't like it :-)

For a fundamental reason or because it happens to not work yet? :)

- z

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Jeff Dike]

On Fri, Jun 08, 2007 at 09:26:01AM -0700, Zach Brown wrote:
> >I don't like it :-)
> 
> For a fundamental reason or because it happens to not work yet? :)

The latter - it fails the test that I posted.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [PATCH] Syslets - Fix cachemiss_thread return value

[Zach Brown]

> The latter - it fails the test that I posted.

OK, good.  That's easy enough to fix :)  I'll send out a tested version.

- z