| |
|
 |
|
 |
|
|
|
| |
|
|
xen-devel
RE: [Xen-devel][Pv-ops][PATCH 0/3 v3] Netback multiple threads support
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
|
|
|
| |
|
Home