Date: Tue, 15 Aug 2006 16:23:30 +0200 From: Suleiman Souhlal <ssouhlal@FreeBSD.org> To: Divacky Roman <xdivac02@stud.fit.vutbr.cz> Cc: emulation@freebsd.org, current@freebsd.org Subject: Re: SoC: linuxolator update: first patch Message-ID: <44E1D8E2.9060200@FreeBSD.org> In-Reply-To: <20060814170418.GA89686@stud.fit.vutbr.cz> References: <20060814170418.GA89686@stud.fit.vutbr.cz>
next in thread | previous in thread | raw e-mail | index | archive | help
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
@@ -93,6 +94,9 @@
#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?44E1D8E2.9060200>