Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 7 Jul 2006 17:28:51 GMT
From:      Roman Divacky <rdivacky@FreeBSD.org>
To:        Perforce Change Reviews <perforce@FreeBSD.org>
Subject:   PERFORCE change 100906 for review
Message-ID:  <200607071728.k67HSphB053593@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=100906

Change 100906 by rdivacky@rdivacky_witten on 2006/07/07 17:28:09

	o	introduce locking using rwlocks
	o	fix bug in linux_clone() regarding pid
	o	introduce em_find()

Affected files ...

.. //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux.h#7 edit
.. //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux_machdep.c#10 edit
.. //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux_sysvec.c#6 edit

Differences ...

==== //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux.h#7 (text+ko) ====

@@ -32,6 +32,9 @@
 #define	_I386_LINUX_LINUX_H_
 
 #include <sys/signal.h> /* for sigval union */
+#include <sys/param.h>
+#include <sys/lock.h>
+#include <sys/rwlock.h>
 
 #include <i386/linux/linux_syscall.h>
 
@@ -769,4 +772,13 @@
 	SLIST_ENTRY(linux_emuldata) emuldatas;
 };
 
+#define EMUL_RLOCK(l)	rw_rlock(l)
+#define EMUL_RUNLOCK(l)	rw_runlock(l)
+#define EMUL_WLOCK(l)	rw_wlock(l)
+#define EMUL_WUNLOCK(l)	rw_wunlock(l)
+
+/* for em_find use */
+#define EMUL_LOCKED		1
+#define EMUL_UNLOCKED		0
+
 #endif /* !_I386_LINUX_LINUX_H_ */

==== //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux_machdep.c#10 (text+ko) ====

@@ -65,10 +65,12 @@
 SLIST_HEAD(emuldata_head, linux_emuldata) emuldata_head = 
 	SLIST_HEAD_INITIALIZER(emuldata_head);
 struct linux_emuldata *emuldata_headp;	/* where we store the emulation data */
+struct rwlock emul_lock;
 
 static int linux_proc_init(struct thread *, pid_t);
 int linux_proc_exit(struct thread *);
 int linux_userret(struct thread *);
+static struct linux_emuldata *em_find(pid_t pid, int locked);
 
 struct l_descriptor {
 	l_uint		entry_number;
@@ -285,6 +287,25 @@
 	return (linux_select(td, &newsel));
 }
 
+/* this returns locked reference to the emuldata entry (if found) */
+static struct linux_emuldata *
+em_find(pid_t pid, int locked)
+{
+	struct linux_emuldata *em;
+
+	if (locked == EMUL_UNLOCKED)
+   		EMUL_RLOCK(&emul_lock);
+
+	SLIST_FOREACH(em, &emuldata_head, emuldatas)
+		if (em->pid == pid)
+			break;
+
+	if (em == NULL && locked == EMUL_UNLOCKED)
+	   	EMUL_RUNLOCK(&emul_lock);
+
+	return (em);
+}
+
 int
 linux_fork(struct thread *td, struct linux_fork_args *args)
 {
@@ -308,10 +329,12 @@
 		return (error);
 
 #if 0	
+	EMUL_RLOCK(&emul_data);
 	/* impossible to not find it */
 	SLIST_FOREACH(em, &emuldata_head, emuldatas)
 	   	if (em->pid == td->td_retval[0])
 		   	break;
+	EMUL_RUNLOCK(&emul_data);
 #endif
 	return (0);
 }
@@ -405,17 +428,21 @@
 		return (error);
 	
 	/* create the emuldata */
-	linux_proc_init(td, td->td_retval[0]);
-	SLIST_FOREACH(em, &emuldata_head, emuldatas)
-		if (em->pid == td->td_retval[0])
-		   	break;
+	error = linux_proc_init(td, p2->p_pid);
+	/* reference it - no need to check this */
+	em = em_find(p2->p_pid, EMUL_UNLOCKED);
+	KASSERT(em != NULL, "no emuldata after proc_init()!\n");
 	/* and adjust it */
 	if (args->flags & CLONE_PARENT_SETTID) {
-	   	if (args->parent_tidptr == NULL)
+	   	if (args->parent_tidptr == NULL) {
+		   	EMUL_RUNLOCK(&emul_lock);
 			return (EINVAL);
+		}
 		error = copyout(&td->td_proc->p_pid, args->parent_tidptr, sizeof(td->td_proc->p_pid));
-		if (error)
+		if (error) {
+		   	EMUL_RUNLOCK(&emul_lock);
 			return (error);
+		}
 	}
 	   	
 	if (args->flags & CLONE_CHILD_CLEARTID)
@@ -427,6 +454,7 @@
 		em->child_set_tid = args->child_tidptr;
 	else
 	   	em->child_set_tid = NULL;
+	EMUL_RUNLOCK(&emul_lock);
 
 	PROC_LOCK(p2);
 	p2->p_sigparent = exit_signal;
@@ -1022,7 +1050,6 @@
 linux_proc_init(struct thread *td, pid_t child)
 {
 	struct linux_emuldata *em, *p_em;
-	int found = 0;
 	struct proc *p;
 
 	/* XXX: locking? */
@@ -1030,16 +1057,13 @@
 	   	/* non-exec call */
    		MALLOC(em, struct linux_emuldata *, sizeof *em, M_LINUX, M_WAITOK | M_ZERO);
 		em->pid = child;
+		EMUL_WLOCK(&emul_lock);
 		SLIST_INSERT_HEAD(&emuldata_head, em, emuldatas);
 	} else {
-	   	found = 0;
 		/* lookup the old one */
-   	   	SLIST_FOREACH(em, &emuldata_head, emuldatas)
-   			if (em->pid == td->td_proc->p_pid) {
-		   		found = 1;
-				break;
-			}
-		if (found == 0) {
+		em = em_find(td->td_proc->p_pid, EMUL_UNLOCKED);
+		/* XXX: this might be turned into KASSERT once its tested enough */
+		if (em == NULL) {
 		   	/* this should not happen */
 #ifdef DEBUG
 		   	printf("emuldata not found in exec case.\n");
@@ -1052,22 +1076,24 @@
 	em->child_set_tid = NULL;
 
 	/* SLIST is inefficient - use hash instead */
-	/* find the emuldata for the parent process */
-   	SLIST_FOREACH(p_em, &emuldata_head, emuldatas)
-   		if (p_em->pid == td->td_proc->p_pid) {
-		   	found = 1;
-			break;
+	/* I hope I rewrote the semantics right */
+	if (child != 0) {
+   	   	/* find the emuldata for the parent process */
+	   	p_em = em_find(td->td_proc->p_pid, EMUL_LOCKED);
+		if (p_em == NULL) {
+		   	em->clear_tid = NULL;
+   		   	em->set_tid = NULL;
+		} else {
+   			em->clear_tid = p_em->clear_tid;
+			em->set_tid = p_em->set_tid;
 		}
-
-	if (found) {
-   		em->clear_tid = p_em->clear_tid;
-		em->set_tid = p_em->set_tid;
+		EMUL_WUNLOCK(&emul_lock);
    	} else {
 	   	em->clear_tid = NULL;
 		em->set_tid = NULL;
+		EMUL_RUNLOCK(&emul_lock);
 	}
 
-
 	/* XXX: sched_lock locking? */
 
 	/* find the newly created thread and set the P_LINUX flag */
@@ -1087,21 +1113,34 @@
 	int error;
 
 	/* find the emuldata */
-	SLIST_FOREACH(em, &emuldata_head, emuldatas)
-		if (em->pid == td->td_proc->p_pid)
-	      		break;
+	em = em_find(td->td_proc->p_pid, EMUL_UNLOCKED);
 
+#ifdef DEBUG
+	if (em == NULL) {
+	   	printf("we didnt find emuldata for the exiting process.\n");
+		return (0);
+	}
+#endif
 	if (em->clear_tid != NULL) {
 		int null = 0;
 
 		error = copyout(&null, em->clear_tid, sizeof(null));
-		if (error)
+		if (error) {
+		   	EMUL_RUNLOCK(&emul_lock);
 		   	return (error);
+		}
 
 		/* TODO: futexes stuff */
 	}
 
+	EMUL_RUNLOCK(&emul_lock);
+	/* XXX: there is a race but I think we can ommit that
+	 * because its not very possible that the same process 
+	 * will exit on different cpus etc.
+	 */
+	EMUL_WLOCK(&emul_lock);
 	SLIST_REMOVE(&emuldata_head, em, linux_emuldata, emuldatas);
+	EMUL_WUNLOCK(&emul_lock);
 
 	/* clean the stuff up */
 	FREE(em, M_LINUX);
@@ -1116,13 +1155,20 @@
 	int error = 0;
 
 	 /* find the emuldata */
-	SLIST_FOREACH(em, &emuldata_head, emuldatas)
-		if (em->pid == td->td_proc->p_pid)
-			break;
+	em = em_find(td->td_proc->p_pid, EMUL_UNLOCKED);
+
+#ifdef DEBUG
+	if (child == NULL) {
+	   	printf("we didnt find emuldata for the userreting process.\n");
+		EMUL_RUNLOCK(&emul_lock);
+		return (0);
+	}
+#endif
 
 	if (em->set_tid != NULL)
 	   	error = copyout(&td->td_proc->p_pid, em->set_tid, sizeof(td->td_proc->p_pid));
 
+	EMUL_RUNLOCK(&emul_lock);
 	return (error);
 }
 
@@ -1132,12 +1178,19 @@
    	struct linux_emuldata *em;
 
 	 /* find the emuldata */
-	SLIST_FOREACH(em, &emuldata_head, emuldatas)
-		if (em->pid == td->td_proc->p_pid)
-			break;
+	em = em_find(td->td_proc->p_pid, EMUL_UNLOCKED);
+
+#ifdef DEBUG
+	if (child == NULL) {
+	   	printf("we didnt find emuldata for the userreting process.\n");
+		EMUL_RUNLOCK(&emul_lock);
+		return (0);
+	}
+#endif
 
 	em->clear_tid = args->tidptr;
 	td->td_retval[0] = td->td_proc->p_pid;
 
+	EMUL_RUNLOCK(&emul_lock);
 	return 0;
 }

==== //depot/projects/soc2006/rdivacky_linuxolator/i386/linux/linux_sysvec.c#6 (text+ko) ====

@@ -111,6 +111,7 @@
 
 extern int linux_userret(struct thread *);
 extern int linux_proc_exit(struct thread *);
+extern struct rwlock emul_lock;
 
 /*
  * Linux syscalls return negative errno's, we do positive and map them
@@ -917,6 +918,7 @@
 		SLIST_INIT(&emuldata_head);
 		linux_proc_exit_p = linux_proc_exit;
 		linux_userret_p = linux_userret;
+		rw_init(&emul_lock, "emuldata lock");
 		break;
 	case MOD_UNLOAD:
 		for (brandinfo = &linux_brandlist[0]; *brandinfo != NULL;
@@ -940,6 +942,7 @@
 			printf("Could not deinstall ELF interpreter entry\n");
 		linux_proc_exit_p = NULL;
 		linux_userret_p = NULL;
+		rw_destroy(&emul_lock);
 		break;
 	default:
 		return EOPNOTSUPP;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200607071728.k67HSphB053593>