Re: SoC: linuxolator update: first patch

On Tuesday 15 August 2006 10:23, Suleiman Souhlal wrote:
> 
> 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.

That is possibly sketchier as if an ABI always sets p_emuldata on fork/exec
and only curthread accesses curproc->p_emuldata you can probably just not have 
to do any locking at all as it will actually be effectively read-only.  The 
emuldata fields can have different strategies in different ABIs because they 
will be used differently.  Note how so_emuldata is locked by SOCK_LOCK() in 
the Linux ABI, but isn't locked in the svr4/streams code because it's set on 
socket creation and static until socket close.

> +		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.

> +		} 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.

See above, but there is another concern here in that once you drop the proc 
lock, the 'p' process might go away, be free'd, etc.  (Especially if you are 
preempted right after the PROC_UNLOCK())

> +
> +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. :)

> ...
> +
> +	pp = p->p_pptr;		/* switch to parent */
> +	PROC_LOCK(pp);
> +	PROC_UNLOCK(p);
> +
> 
> You might have to hold the proctree_lock here.

Actually, for this he is fine, as the proc lock is sufficient to access 
p->p_pptr, and the parent process can't go away w/o acquiring the child's 
lock and reparenting it to init.

-- 
John Baldwin