Re: [Xen-devel][Pv-ops][PATCH 0/3] Resend: Netback multiple thread support

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel