Date: Tue, 15 Aug 2006 17:20:47 +0200 From: Suleiman Souhlal <ssouhlal@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: emulation@freebsd.org, Divacky Roman <xdivac02@stud.fit.vutbr.cz>, freebsd-current@freebsd.org Subject: Re: SoC: linuxolator update: first patch Message-ID: <44E1E64F.6020205@FreeBSD.org> In-Reply-To: <200608151101.30951.jhb@freebsd.org> References: <20060814170418.GA89686@stud.fit.vutbr.cz> <44E1D8E2.9060200@FreeBSD.org> <200608151101.30951.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
John Baldwin wrote:
>
>>+ 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.
>
>
> Well, in this case he's already holding a lock. If he always holds a lock
> when accessing and modifying refs, then refcount_*() would only add overhead.
Isn't he holding the wrong lock (emul_lock vs emul_shared_lock)?
>>+
>>+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?
>
>
> In this case it is a workaround for lost wakeups since it's not an interlocked
> sleep and wakeup. :)
Ew..
Thanks,
-- Suleiman
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?44E1E64F.6020205>