On Thu, 21 Jun 2007 14:53:20 +0100
Andrew Doran <ad@netbsd.org> wrote:

> 1. sysmon_envsys_find()
> 
> 	mutex_enter(&sme_mtx);
> 	LIST_FOREACH(sme, &sysmon_envsys_list, sme_list) {
> 		if (strcmp(sme->sme_name, name) == 0)
> 			break;
> 	}
> 	mutex_exit(&sme_mtx);
> 	return sme;
> 
> How do you know the item is still there after you drop sme_mtx? Can it be
> taken off the list as soon as you unlock?

I check in the parts that use sysmon_envsys_find() if returned is NULL,
isn't it enough? or are you talking about something else?

> 2. In sme_make_dictionary(), why acquire and release the lock so many
> times? Wouldn't it be clearer to acquire it fewer times?

I was using the approach that you said, but rmind@ suggested to avoid
doing large operations with a mutex held...

> 3. sme_event_unregister():
> 
> 	if (LIST_EMPTY(&sme_events_list)) {
> 		mutex_exit(&sme_event_mtx);
> 		callout_stop(&seeco);
> 		workqueue_destroy(seewq);
> 
> 		mutex_enter(&sme_event_mtx);
> 		sme_events_initialized = false;
> 
> When you unlock to destroy the work queue / callout, it's possible for
> another thread to do sme_events_init() at the same time, right?

Hmm why? sme_events_init() is protected by the mutex sme_events_init.

> The problem with holding a lock around those is that later on when
> interrupt handlers have thread context, you may want to acquire the lock
> from an interrupt handler or callout. Those cannot be delayed for long.
> Functions like workqueue_create() may want to sleep long term for memory.
> One solution may be to have two locks. One short term lock for data
> (sme_event_mtx) and one longer term lock for sme_events_initialized
> (sme_event_init_mtx) that you hold while setting up or destroying the
> resources.

Also, rmind@ said that I must not allocate/deallocate resources with
an adaptive mutex, is it true? or are you suggesting to protect these
operations with another mutex?

> 4. sme_events_check()
> 
> Since it's a callout you can't take sme_event_mtx there.. It might be
> useful to add some commented out code to that effect, as later it will be
> possible to take the lock, eg:
> 
> sme_events_check(void *arg)
> {
> 	sme_event_t *see;
> 
> 	/* mutex_enter(&sme_event_mtx); XXX notyet */
> 	LIST_FOREACH(see, &sme_events_list, see_list) {

Hmm, ok.

-- 
Juan Romero Pardines	- The NetBSD Project
http://plog.xtrarom.org	- NetBSD/pkgsrc news in Spanish