SoC: help with LISTs and killing procs

[John Baldwin]

On Thursday 10 August 2006 12:17, Divacky Roman wrote:
> On Thu, Aug 10, 2006 at 10:43:05AM -0500, Brooks Davis wrote:
> > On Thu, Aug 10, 2006 at 05:35:43PM +0200, Divacky Roman wrote:
> > > On Thu, Aug 10, 2006 at 10:23:59AM -0500, Brooks Davis wrote:
> > > > On Thu, Aug 10, 2006 at 05:16:17PM +0200, Divacky Roman wrote:
> > > > > hi
> > > > > 
> > > > > I am doing this:
> > > > > 
> > > > > (pseudocode)
> > > > > LIST_FOREACH_SAFE(em, &td_em->shared->threads, threads, tmp_em) {
> > > > > 
> > > > > 	kill(em, SIGKILL);
> > > > > }
> > > > > 
> > > > > kill(SIGKILL) calls exit() which calls my exit_hook()
> > > > > 
> > > > > my exit_hook() does LIST_REMOVE(em, threads).
> > > > > 
> > > > > the problem is that this is not synchronous so I am getting a panic 
by INVARIANTS
> > > > > that "Bad link elm prev->next != elm". This is because I list 1st 
item in the list
> > > > > I call kill on it, then process 2nd list, then scheduler preempts my 
code and calls
> > > > > exit() on the first proc which removes the first entry and bad 
things happen. 
> > > > > 
> > > > > I see this possible solutions:
> > > > > 
> > > > > make this synchronous, it can be done by something like:
> > > > > 
> > > > >     ....
> > > > >     kill(em, SIGKILL);
> > > > >     wait_for_proc_to_vanish();
> > > > > 
> > > > > pls. tell me what do you think about this solution and if its 
correct what is the wait_for_proc_to_vanish()
> > > > > 
> > > > > maybe there's some better solution, pls tell me.
> > > > 
> > > > It sounds like you need a lock protecting the list.  If you held it 
over
> > > > the whole loop you could signal all processes before the exit_hook 
could
> > > > remove any.
> > > 
> > > I dont understand. I am protecting the lock by a rw_rlock();
> > > 
> > > the exit_hook() then acquires rw_wlock(); when removing the entry.
> > > what exactly do you suggest me to do? I dont get it.
> > 
> > This can't be the case.  If you're holding a read lock around the
> > loop (it must cover the entire loop), it should not be possible for the
> > exit_hook() to obtain a write lock while you are in the loop.  Just to
> > verify, is the lock for the list and not per element?
> 
> oh.. I see whats going on.. in the exit_hook I am doing this:
> 
> 
>      	em = em_find(p->p_pid, EMUL_UNLOCKED);	// this performs 
EMUL_RLOCK(&emul_lock);
>      	...
> 	EMUL_RUNLOCK(&emul_lock);
> 	
> 	EMUL_WLOCK(&emul_lock);
> 	LIST_REMOVE(em, threads);
> 	SLIST_REMOVE(&emuldata_head, em, linux_emuldata, emuldatas);
> 	EMUL_WUNLOCK(&emul_lock);
> 									  
> the EMUL_RUNLOCK() unlocks it so it doesnt wait. This should be turned into 
rw_try_upgrade()
> but I dont understand how ;(

You could make em_find() take an argument to specify if it should acquire
a WLOCK instead of an RLOCK.  Really, unless you have measured a lot of 
contention on this lock, you should just make it a mtx, and only go back and 
make it a rwlock if it really needs it.  Also, you currently can't do an 
interlocked msleep() or cv_wait() with a rwlock, so you really need to use a 
mutex anyway.

> anyway, I still dont understand how should I use the lock to achieve the 
synchronization.
> 
> my code looks like:
> 
> 	EMUL_RLOCK(&emul_lock);
>       	LIST_FOREACH_SAFE(em, &td_em->shared->threads, threads, tmp_em) {
> 	}
> 	EMUL_RUNLOCK(&emul_lock);
> 
> what do you suggest? I need to process the list first and then let the 
exit_hook in the various processes run.

This is not safe anyway.  kill() is way too big of a function to call with a 
lock held.  You also pass the wrong thread to it IIRC (you should always pass 
curthread as the td argument to a syscall).  You probably need to use 
psignal, and you probably should be doing something like this:

	EMUL_LOCK();
	LIST_FOREACH(td, &td_em->shared->threads, threads) {
		p = td->td_proc;
		PROC_LOCK(p);
		psignal(p, SIGKILL);
		PROC_UNLOCK(p);
	}

	while (THREADS_STILL_AROUND(&td->em))
		msleep(td_em, &emul_lock, PWAIT, "foo", 0);
	EMUL_UNLOCK();

Then in your exit_hook you should do this:

	em = em_find(p->p_pid, EMUL_UNLOCKED);
	LIST_REMOVE(...);
	SLIST_REMOVE(...);
	wakeup(em);
	EMUL_UNLOCK();

-- 
John Baldwin