svn commit: r186187 - head/sys/net

[John Baldwin]

On Wednesday 17 December 2008 3:09:55 pm Max Laier wrote:
> As the upper half of this thread has turned into a style(9) bikeshed, let me 
> replay the lower half and add more locking folks.
> 
> The change in question is here: 
> http://svn.freebsd.org/viewvc/base/head/sys/net/pfil.c?r1=173904&r2=186187 
> 
> On Tuesday 16 December 2008 19:20:40 Robert Watson wrote:
> > On Tue, 16 Dec 2008, Max Laier wrote:
> ...
> > >>   - Don't lock the hook during registration, since it's not yet in use.
> > >
> > > Shouldn't the WLOCK be obtained regardless in order to obtain a memory
> > > barrier for the reading threads?  pfil_run_hooks doesn't interact with
> > > the LIST_LOCK so it's unclear in what state the hook head will be when
> > > the hook head is first read.
> >
> > Hmm.  Interesting observation.  However, that approach is not consistent
> > with lots of other code, so possibly that other code needs to change as
> > well.  For example, ip_init() initializes lots of other global data
> > structures, including the fragment reassembly queue and protocol switch,
> > without acquiring locks in such a way as to force visibility on other 
CPUs.
> >
> > I've CC'd John Baldwin, who might be able to comment on this issue more
> > generally.  Should we be, for example, calling { IPQ_LOCK(); IPQ_UNLOCK();
> > } after initializing the IP reassembly queues to make sure that
> > initialization is visible to other CPUs that will be calling IPQ_LOCK()
> > before using it?

So the usual model would be to do something like this:

	LIST(foo, ) foo_list;

foo_add(foo *p)
{

	/* Construct foo, but don't lock it. */
	WLOCK(foo_list);
	LIST_INSERT(foo_list, foo);
	WUNLOCK(foo_list);
}

foo *
foo_find(int key)
{
	foo *p;

	RLOCK(foo_list);
	LIST_FOREACH(p, foo_list) {
		if (p->key == key) {
			LOCK(p);
			RUNLOCK(foo_list);
			return (p);
		}
	}
	RUNLOCK(foo_list);
	return (NULL);
}
		
something_else()
{
	foo *p;

	RLOCK(foo_list);
	LIST_FOREACH(p, foo_list) {
		LOCK(p);
		bar(p);
		UNLOCK(p);
	}
	RUNLOCK(foo_list);
}

From your description it would appear that you are doing something_else() but 
without actually locking foo_list.  Unless you can demonstrate a clear win in 
benchmarks from not having the lock there, I would suggest just going with 
the simpler and more common approach.  Depending on the mtx of the individual 
head doesn't do anything to solve races with removing the head from the list.

So, reading a bit further, I think the real fix is that the pfil API is a bit 
busted.  What you really want to do is have pfil_get_head() operate like 
foo_find() and lock the head before dropping the list lock.  That is the only 
race I see in the current code.  However, because you malloc() after calling 
pfil_get_head() that is problematic.  That is, the current consumer code 
looks like this:

	ph = pfil_get_head(x, y);

	pfil_add_hook(..., ph);		/* calls malloc() */

I think you should change the API to be this instead:

	pfil_add_hook(..., x, y);

and have pfil_get_head() do the lookup internally:

pfil_add_hook()
{

	malloc(...);

	ph = pfil_get_head(x, y);	/* locks the 'ph' like foo_find() */
}

-- 
John Baldwin