On Tue, Apr 22, 2008 at 10:38:31AM -0700, vijay singh wrote: > > > ----- Original Message ---- > From: Robert Watson <[EMAIL PROTECTED]> > To: Brooks Davis <[EMAIL PROTECTED]> > Cc: vijay singh <[EMAIL PROTECTED]>; freebsd-net@freebsd.org > Sent: Sunday, April 20, 2008 2:27:15 AM > Subject: Re: Regarding if_alloc() > > > On Fri, 18 Apr 2008, Brooks Davis wrote: > > > On Thu, Apr 17, 2008 at 06:35:23PM -0700, vijay singh wrote: > >> Hi all. How do we avoid a race in populating the ifindex_table? Id this is > >> a TODO, as it seems from the code below, would it be acceptable if I wrote > >> a patch and reused the ifnet_lock [IFNET_WLOCK, IFNET_WUNLOCK]? > > > > Locking if_index management with ifnet_lock should be ok. Ideally we > > should > > probably be using ALLOC_UNR(9) to manage if_indexes instead of this rather > > expensive loop. > > > > Be aware, that if_index generation is least of the issues in this area. The > > if_grow() call is much riskier since it changes the value of the global > > ifnet pointer which I'm not sure we can afford to lock. It would be worth > > experimenting with rmlocks to see what the impact if of locking would be. > > > > I'm serious tempted to kill if_grow in favor of some sort of if_index_max > > tunable. > > I've seen a number of reports of panics that may well be traceable to races > in > if_index handling, and have looked a bit at possible fixes. Quite a few uses > of if_index are inherently racy, as they rely on stability of the index > value, > which of course can't be guaranteed with removable interfaces. > > I think a reasonable interim fix would be to protect all use of the byindex > arrays using the ifnet lock, but remember that the read methods, not just the > write methods, need protection, and as such should move from being macros in > if_var.h to functions in if.c. if_grow is probably OK if this is done right, > but it will need to be set up to drop the lock, grow, re-aquire, and > re-validate assumptions (i.e., repeat the search for a free index and loop if > it fails to find one). Once the read methods are using the lock also, we > should seriously consider converting it to an rwlock. We can probably also > un-publicize at least one of the byindex lookup routines (the dev lookup, > which is needed only in if.c). > > This would prevent races on modifying and evaluating the index array, but not > on disappearing cdevs and ifnets, which are a separate problem, and one that > probably is exercised significanty less. The reports I've seen appear to > have > only to do with pulling the array out from other consumers while in use. > > > >> > > Robert, I am working on the patch, but I had a few questions. If we > drop the lock while if_grow() is running, how do we prevent a second > caller, blocked in if_alloc() to start scanning the ifindex_table, and > end up deciding to grow it again. In other words, we need to stall > the ifindex_table scan in if_alloc() till if_grow() has achieved > finished. Since if_grow() will be called relatively infrequently, > should we consider holding the lock through the routine? Your comments > and suggestions are welcome.
You can't hold locks over malloc calls. -- Brooks
pgpWdKNIdhOK4.pgp
Description: PGP signature
