From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [Pv-ops][PATCH 0/3 v3] Netback multiple threads support Date: Tue, 04 May 2010 10:38:35 +0100 Message-ID: <4BE0073B02000078000011E7@vpn.id2.novell.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Dongxiao Xu Cc: Jeremy Fitzhardinge , "xen-devel@lists.xensource.com" , Steven Smith , KonradRzeszutek Wilk List-Id: xen-devel@lists.xenproject.org >>> "Xu, Dongxiao" 04.05.10 03:52 >>> >1. Merge "group" and "idx" into "netif->mapping", therefore >page_ext is not used now. Open coding this seems very fragile (and even more using literal constants in those code fragments). I'm also not convinced restricting either part to 16 bits is a good thing (particularly on 64-bits, where you could easily have each part use 32 bits). >2. Put netbk_add_netif() and netbk_remove_netif() into >__netif_up() and __netif_down(). An extra comment on top of what I said earlier: I don't think holding the lock for the entire duration of the loop in netbk_add_netif() is necessary - not doing so just makes the balancing slightly imprecise, but reduces (on large systems) the spin lock holding time (perhaps significantly). If you don't, and if you instead make the netfront_count member an atomic_t, you can eliminate the lock altogether I would think. >4. Use __get_free_pages() to replace kzalloc(). This is only marginally better than kzalloc(). Why not vmalloc(), as suggested? >6. Use MODPARM_netback_kthread to determine whether using >tasklet or kernel thread. As a minor space optimization, the tasklet and kthread related fields in struct xen_netbk could be put in a union. >8. Add more checks in netif_page_release(). Actually, isn't BUG_ON(!PageForeign(page)); checking just a subset of BUG_ON(netbk->mmap_pages[idx] !=3D page); as all pages in those arrays would always be foreign? Jan