RE: [Xen-devel][Pv-ops][PATCH 0/3 v3] Netback multiple threads support

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

Jan Beulich wrote:
>>>> "Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx> 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).

Do you have any suggestion on how to embed the data into
netif->mapping? 

> 
>> 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.

I will modify it in next version of patch.

> 
>> 4. Use __get_free_pages() to replace kzalloc().
> 
> This is only marginally better than kzalloc(). Why not vmalloc(), as
> suggested?

OK.

> 
>> 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.

OK.

> 
>> 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] != page);
> 
> as all pages in those arrays would always be foreign?

Yes, you are right. That check is redundant and could be removed.

Thanks,
Dongxiao

> 
> Jan


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