[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


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

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


> Jan

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.