Re: SoC: linuxolator update: first patch

Divacky Roman wrote:
> Hi,
> 
> I am a student doing SoC linuxolator update. The work involved updating
> linuxolator to be able to work with 2.6.x (2.6.16 in my case) kernel emulation.
> 
> To be able to run 2.6.x emulation I had to implement NPTL, which means NPTL, futexes
> and thread area, pid mangling + various hacks to make it work.
> 
> This is the first patch meant for public revision/testing:
> 
> 	www.stud.fit.vutbr.cz/~xdivac02/linuxolator26-diff.patch

Very nice work! You can find some comments below.

---  amd64/linux32/linux.h.orig
+++  amd64/linux32/linux.h
...
+
+struct l_desc_struct {
+           unsigned long a,b;
+};
+

Please change this so that the two fields are on separate lines.
Also, is there a reason you're using "unsigned long" and not "l_ulong"?
On amd64 long are 64bit long while on i386 they are 32 bits, so there 
might be problems, if you're trying to 'emulate' 32bit linux programs on 
amd64.

+
+#define LINUX_LOWERWORD	0x0000ffff
+

Technically, it's the lower half-word.

---  amd64/linux32/linux32_machdep.c.orig
+++  amd64/linux32/linux32_machdep.c
...
-	td2->td_frame->tf_rsp = PTROUT(args->stack);
+	/* in a case of stack = NULL we are supposed to COW calling process stack
+	 * this is what normal fork() does so we just keep the tf_rsp arg intact
+	 */
+	if (args->stack)
+   	   	td2->td_frame->tf_rsp = PTROUT(args->stack);

style(9) says multi-line comments have to start with /* on a line of its 
own. Please fix all the instances of this.

--- /dev/null	Mon Aug 14 18:44:00 2006
+++ compat/linux/linux_emul.c	Mon Aug 14 18:46:20 2006
+
+struct sx emul_shared_lock;
+struct sx emul_lock;

Are you using sx instead of rwlocks because you need to sleep in 
linux_schedtail() while holding the lock?

+
+/* this returns locked reference to the emuldata entry (if found) */
+struct linux_emuldata *
+em_find(struct proc *p, int locked)
+{
+	struct linux_emuldata *em;
+
+	if (locked == EMUL_UNLOCKED)
+   		EMUL_LOCK(&emul_lock);
+
+	em = p->p_emuldata;	   	
+
+	if (em == NULL && locked == EMUL_UNLOCKED)
+	   	EMUL_UNLOCK(&emul_lock);
+
+	return (em);
+}
+
+int
+linux_proc_init(struct thread *td, pid_t child, int flags)
+{
+	struct linux_emuldata *em, *p_em;
+	struct proc *p;
+
+	if (child != 0) {
+	   	/* non-exec call */
+   		MALLOC(em, struct linux_emuldata *, sizeof *em, M_LINUX, M_WAITOK 
| M_ZERO);
+		em->pid = child;
+		if (flags & CLONE_VM) {
+		   	/* handled later in the code */
+		} else {
+		   	struct linux_emuldata_shared *s;
+
+   			MALLOC(s, struct linux_emuldata_shared *, sizeof *s, M_LINUX, 
M_WAITOK | M_ZERO);
+			em->shared = s;
+			s->refs = 1;
+			s->group_pid = child;
+
+			LIST_INIT(&s->threads);
+		}
+		p = pfind(child);
+		if (p == NULL)
+		   	panic("process not found in proc_init\n");

Why not use KASSERT(p != NULL, ("process not found in proc_init\n")); ?

+		p->p_emuldata = em;
+		PROC_UNLOCK(p);
+	} else {
+		/* lookup the old one */
+		em = em_find(td->td_proc, EMUL_UNLOCKED);

According to sys/proc.h you're supposed to hold the proc lock when 
accessing p->p_emuldata.

+		KASSERT(em != NULL, ("proc_init: emuldata not found in exec case.\n"));
+	}
+
+	em->child_clear_tid = NULL;
+	em->child_set_tid = NULL;
+
+	/* allocate the shared struct only in clone()/fork cases
+	 * in the case of clone() td = calling proc and child = pid of
+	 * the newly created proc
+	 */
+	if (child != 0) {
+   	   	if (flags & CLONE_VM) {
+   		   	/* lookup the parent */
+		   	p_em = em_find(td->td_proc, EMUL_LOCKED);
+			KASSERT(p_em != NULL, ("proc_init: parent emuldata not found for 
CLONE_VM\n"));
+			em->shared = p_em->shared;
+			em->shared->refs++;

This is unsafe. Please use the functions in sys/refcount.h.

+		} else {
+		   	/* handled earlier to avoid malloc(M_WAITOK) with rwlock held */
+		}
+	}
+
+
+	if (child != 0) {
+	   	EMUL_SHARED_WLOCK(&emul_shared_lock);
+   	   	LIST_INSERT_HEAD(&em->shared->threads, em, threads);
+	   	EMUL_SHARED_WUNLOCK(&emul_shared_lock);
+
+		p = pfind(child);
+		PROC_UNLOCK(p);
+		/* we might have a sleeping linux_schedtail */
+		wakeup(&p->p_emuldata);

Again, you need to hold the proc lock when accessing p->p_emuldata.

+	} else
+	   	EMUL_UNLOCK(&emul_lock);
+
+   	return (0);
+}
+
+void
+linux_proc_exit(void *arg __unused, struct proc *p)
+{
+   	struct linux_emuldata *em;
+	int error;
+	struct thread *td = FIRST_THREAD_IN_PROC(p);

Don't you need to hold sched_lock to access p->p_threads?

+	int *child_clear_tid;
+
+	if (__predict_true(p->p_sysent != &elf_linux_sysvec))
+	   	return;
+
+	/* find the emuldata */
+	em = em_find(p, EMUL_UNLOCKED);

proc lock.. :-)

+
+	KASSERT(em != NULL, ("proc_exit: emuldata not found.\n"));
+
+	child_clear_tid = em->child_clear_tid;
+	
+	EMUL_UNLOCK(&emul_lock);
+
+	EMUL_SHARED_WLOCK(&emul_shared_lock);

Is this safe?

+	LIST_REMOVE(em, threads);
+
+	PROC_LOCK(p);
+	p->p_emuldata = NULL;
+	PROC_UNLOCK(p);
+
+	em->shared->refs--;
+	if (em->shared->refs == 0)

sys/refcount.h

+		FREE(em->shared, M_LINUX);
+	EMUL_SHARED_WUNLOCK(&emul_shared_lock);
+
+	if (child_clear_tid != NULL) {
+	   	struct linux_sys_futex_args cup;
+		int null = 0;
+
+		error = copyout(&null, child_clear_tid, sizeof(null));
+		if (error)
+		   	return;

You're forgetting to free em before returning.

+
+		/* futexes stuff */
+		cup.uaddr = child_clear_tid;
+		cup.op = LINUX_FUTEX_WAKE;
+		cup.val = 0x7fffffff; /* Awake everyone */
+		cup.timeout = NULL;
+		cup.uaddr2 = NULL;
+		cup.val3 = 0;
+		error = linux_sys_futex(FIRST_THREAD_IN_PROC(p), &cup);
+		/* this cannot happen at the moment and if this happens
+		 * it probably mean there is a userspace bug
+		*/

Wrong indentation.

+		if (error)
+		   	printf(LMSG("futex stuff in proc_exit failed.\n"));
+	}
...
+
+extern int hz;		/* in subr_param.c */
+

Why don't you include sys/time.h?

+
+void
+linux_schedtail(void *arg __unused, struct proc *p)
+{
+	struct linux_emuldata *em;
+	int error = 0;
+#ifdef	DEBUG
+	struct thread *td = FIRST_THREAD_IN_PROC(p);
+#endif
+	int *child_set_tid;
+
+	if (p->p_sysent != &elf_linux_sysvec)
+	   	return;
+
+retry:	
+	/* find the emuldata */
+	em = em_find(p, EMUL_UNLOCKED);
+
+	if (em == NULL) {
+	   	/* We might have been called before proc_init for this process so
+		 * tsleep and be woken up by it. We use p->p_emuldata for this
+		 */
+
+	   	error = tsleep(&p->p_emuldata, PLOCK, "linux_schedtail", hz);
+		if (error == 0)
+		   	goto retry;

Why are you setting a timeout if you just retry when it expires?

+	   	panic("no emuldata found for userreting process.\n");
+	}
+	child_set_tid = em->child_set_tid;
+	EMUL_UNLOCK(&emul_lock);
+
+	if (child_set_tid != NULL)
+	   	error = copyout(&p->p_pid, (int *) child_set_tid, sizeof(p->p_pid));
+
+	return;
+}

--- compat/linux/linux_misc.c.orig
+++ compat/linux/linux_misc.c
_at__at_ -93,6 +94,9 _at__at_
  #define BSD_TO_LINUX_SIGNAL(sig)	\
  	(((sig) <= LINUX_SIGTBLSZ) ? bsd_to_linux_signal[_SIG_IDX(sig)] : sig)

+extern struct sx emul_shared_lock;
+extern struct sx emul_lock;
+

Please move these to compat/linux/linux_emul.h

...
+
+	pp = p->p_pptr;		/* switch to parent */
+	PROC_LOCK(pp);
+	PROC_UNLOCK(p);
+

You might have to hold the proctree_lock here.

--- compat/linux/linux_signal.c.orig
+++ compat/linux/linux_signal.c
+	if (args->tgid == -1)
+	   	return linux_kill(td, &ka);
+
+	if ((p = pfind(args->pid)) == NULL)
+	      	return ESRCH;
+
+	if (p->p_sysent != &elf_linux_sysvec)
+		return ESRCH;

Please use return (...);


-- Suleiman