Skip to contentSkip to conversation

Syslets - Fix cachemiss_thread return value

PATCH In review Jeff Dike 3 participants 10 messages May 31, 2007 – Jun 8, 2007 User-mode Linux (UML)

lore
Share & tools
lore

Jeff Dike

author
#0 lore raw

PATCH 1/1 Syslets - Fix cachemiss_thread return value

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 <[email protected]>

--

 kernel/async.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/async.c +2 -2
Index: linux-2.6/kernel/async.c
===================================================================
@@ -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;
 }
 
 /**

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

#1 lore raw
Quoted 3 lines · 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.

oops, indeed! Thanks.

Ingo

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

#2 lore raw
In reply to Jeff Dike
Quoted 3 lines

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

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

author
#3 lore raw
Quoted 11 lines · Zach Brown

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.

Quoted 3 lines

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

Signature · 11 lines
Work email - jdike at linux dot intel dot com

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
#4 lore raw
Quoted 2 lines

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

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

#5 lore raw
In reply to Jeff Dike
Quoted 3 lines · 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.

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 <[email protected]>
kernel/async.c +31 -15
diff -r f0d8ee165e2e kernel/async.c
@@ -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);
 }
 
 /**

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

author
#6 lore raw
Quoted 7 lines · Zach Brown
Quoted 3 lines · 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.

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

Signature · 172 lines
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;
}

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
#7 lore raw
Quoted 1 line

I don't like it :-)

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

- z

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

author
#8 lore raw
Quoted 3 lines · Zach Brown
Quoted 1 line

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

Signature · 11 lines
Work email - jdike at linux dot intel dot com

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
#9 lore raw
Quoted 1 line

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

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel