From owner-freebsd-net@FreeBSD.ORG Tue Apr 22 17:38:32 2008 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 310031065672 for ; Tue, 22 Apr 2008 17:38:32 +0000 (UTC) (envelope-from vijjus@rocketmail.com) Received: from web33502.mail.mud.yahoo.com (web33502.mail.mud.yahoo.com [68.142.206.151]) by mx1.freebsd.org (Postfix) with SMTP id E03B68FC12 for ; Tue, 22 Apr 2008 17:38:31 +0000 (UTC) (envelope-from vijjus@rocketmail.com) Received: (qmail 20728 invoked by uid 60001); 22 Apr 2008 17:38:31 -0000 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=rocketmail.com; h=X-YMail-OSG:Received:X-Mailer:Date:From:Subject:To:Cc:MIME-Version:Content-Type:Message-ID; b=S0wvPjaMfotF/ECyLHvlJ4Hcpo39A02fpkoAvvCIaK0LCNFJlyGgV/+4sbu0CPx4Bsy95Cb1rQYxAju7jbJGiXjP1axdrW+0QsV3sZhRkA3qnteKv0ICpHMXq8k0oPmk0NBqhRd8SH83VwCbSQ+CbQDCqY8DGJIwjZzzMEUwInY=; X-YMail-OSG: C_aUJWwVM1lVI9pZk.pBmBj7TOzjAIy4Qu3myfP8wk.E87MzCcs7aYgDhTisG27jiQ-- Received: from [198.95.226.230] by web33502.mail.mud.yahoo.com via HTTP; Tue, 22 Apr 2008 10:38:31 PDT X-Mailer: YahooMailRC/902.40 YahooMailWebService/0.7.185 Date: Tue, 22 Apr 2008 10:38:31 -0700 (PDT) From: vijay singh To: Robert Watson , Brooks Davis MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Message-ID: <340035.20572.qm@web33502.mail.mud.yahoo.com> Cc: freebsd-net@freebsd.org Subject: Re: Regarding if_alloc() X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Apr 2008 17:38:32 -0000 ----- Original Message ---- From: Robert Watson To: Brooks Davis Cc: vijay singh ; 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. -vijay ____________________________________________________________________________________ Be a better friend, newshound, and know-it-all with Yahoo! Mobile. Try it now. http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ