From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [Pv-ops][PATCH 0/3] Resend: Netback multiple thread support Date: Tue, 27 Apr 2010 11:37:08 -0700 Message-ID: <4BD72ED4.5060409@goop.org> References: <4FA716B1526C7C4DB0375C6DADBC4EA342A7A7E95E@LONPMAILBOX01.citrite.net> <4B182D87.6030901@goop.org> <4B187513.80003@goop.org> <4B200727.8040000@goop.org> <4B213766.7030201@goop.org> <20100427104925.GA14523@weybridge.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20100427104925.GA14523@weybridge.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Steven Smith Cc: "Xu, Dongxiao" , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 04/27/2010 03:49 AM, Steven Smith wrote: >> I'd like to make an update on these patches. The main logic is not >> changed, and I only did a rebase towards the upstream pv-ops kernel. >> See attached patch. The original patches are checked in Jeremy's >> netback-tasklet branch. >> > I have a couple of (quite minor) comments on the patches: > > 0001-Netback-Generilize-static-global-variables-into-stru.txt: > >> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c >> index c24debf..a484b0a 100644 >> --- a/drivers/xen/netback/netback.c >> +++ b/drivers/xen/netback/netback.c >> @@ -49,18 +49,13 @@ >> >> /*define NETBE_DEBUG_INTERRUPT*/ >> >> +struct xen_netbk *xen_netbk; >> > >> +int group_nr = 1; >> +struct page_foreign_tracker *foreign_page_tracker; >> > I think these two would benefit from more descriptive names, given > that they're not static. > > Yes. Actually I thought I raised the same points the first time through and Dongxiao had posted patches addressing them. I have to admit I haven't looked at the reposted patches in detail yet. Have we suffered a regression here? Hm, maybe its just this issue which slipped through. > If I was feeling pedantic, I'd complain that this includes some bits > of support for multiple struct xen_netbks, rather than just moving all > of the fields around, which reduces its obviously-correct-ness quite a > bit. > I'm always in favour of having more, smaller, functionally distinct patches, so if this patch can be further subdivided then it would be nice to do so. It's especially useful when trying to track down regressions via bisection. > Even more pedantically, it might be better to pass around a struct > xen_netbk in a few places, rather than an int group, so that you get > better compiler type checking. > +1 > Apart from that, it all looks fine to me. > Thanks for looking at this. It had been missing the gaze of some networking-savvy eyes. J