svn commit: r186187 - head/sys/net

[Max Laier]

Thank you for getting back to this ...

On Thursday 15 January 2009 22:59:35 John Baldwin wrote:
> 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);

Is this allowed now?  I was under the impression that it was bad for some 
reason to interchange locks/unlocks like this.  Something about WITNESS 
getting confused and/or real lock order problems?  I'm more than happy if 
that's not the case.

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

The thing is that the list of pfil_heads is just a "configuration time" 
construct.  We really don't want to look at it in the hot path.  Hence we 
allow the caller of pfil_head_register() (foo_add) to hold on to its reference 
of the pfil_head and work with it without requiring any interaction with the 
lookup list.  This, however, is not the problem at all.  It's the 
responsibility of the caller to ensure that it won't fiddle with the cached 
reference after calling pfil_head_unregister().

> 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() */
> }

This is a good idea/catch, but still not my initial problem.

My real problem is what foo_remove() should look like in the scenario above.

I assume that it should look something like this:

foo_remove(int key) {

  WLOCK(foo_list);
  LIST_FOREACH(p, foo_list)
    if (p->key == key) {
       LIST_REMOVE(p, entries);

       LOCK(p);         /* <--- HERE */

       WUNLOCK(foo_list);
       /* free resources inside p */
       /* uninit lock */
       free(p);
       return;
    }
  WUNLOCK(foo_list);

}
       
I assume that locking the element's lock above is necessary as a pfil_add_hook 
as you describe above will only hold the element's lock while adding resources 
to it and not the list lock (as foo_find drops that before returning) and as 
such we might not see all changes done by pfil_add_hook in the thread that is 
doing the foo_remove(), right?

Something similar is true for the the foo_add() above.  Is there a reason why 
we don't want to lock the element's lock while we initialize?  While it is 
safe if we always go through foo_find() before altering the element and thus 
synchronize on the list lock, it seems bogus to rely on this (even though it 
does make sense to go through foo_find() every time as I wrote earlier).

-- 
/"\  Best regards,                      | mlaier at freebsd.org
\ /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.love2party.net/  | mlaier at EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News