Re: SoC: linuxolator update: first patch

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