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

The patchset is against xen/master tree. It seems some merges
in xen/master/netback is not contained in xen/next/netback,
for example, the foreign_page_tracker.

Main Changes from v2:
1. Merge "group" and "idx" into
"netif->mapping", therefore
page_ext is not used now.
2. Put netbk_add_netif() and netbk_remove_netif() into
__netif_up() and __netif_down().
3. Change the usage of kthread_should_stop().
4. Use __get_free_pages() to replace kzalloc().
5. Modify the changes to netif_be_dbg().
6. Use MODPARM_netback_kthread to determine whether using
tasklet or kernel thread.
7. Put small fields in the front, and large arrays in the end of struct
xen_netbk.
8. Add more checks in netif_page_release().

Current netback uses one pair of tasklets for Tx/Rx data transaction.
Netback tasklet could only run at one CPU at a time, and it is used to
serve all the netfronts. Therefore it has become a performance bottle
neck. This patch is to use multiple tasklet pairs to replace the current
single pair in dom0. 

Assuming that Dom0 has CPUNR VCPUs, we define CPUNR kinds of
tasklets pair (CPUNR for Tx, and CPUNR for Rx). Each pare of tasklets
serve specific group of netfronts. Also for those global and static
variables, we duplicated them for each group in order to avoid the
spinlock. 

PATCH 01: Generilize static/global variables into ''struct
xen_netbk''.

PATCH 02: Multiple tasklets support.

PATCH 03: Use Kernel thread to replace the tasklet.

Recently I re-tested the patchset with Intel 10G multi-queue NIC device,
and use 10 outside 1G NICs to do netperf tests with that 10G NIC.

Case 1: Dom0 has more than 10 vcpus pinned with each physical CPU.
With the patchset, the performance is 2x of the original throughput.

Case 2: Dom0 has 4 vcpus pinned with 4 physical CPUs.
With the patchset, the performance is 3.7x of the original throughput. 

when we test this patch, we found that the domain_lock in grant table
operation (gnttab_copy()) becomes a bottle neck. We temporarily
remove the global domain_lock to achieve good performance.

Thanks,
Dongxiao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 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).
>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.
>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?

Jan


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

Jan Beulich wrote:
>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Xu, Dongxiao" <dongxiao.xu@intel.com> 04.05.10
13:36 >>>
>Jan Beulich wrote:
>>>>> "Xu, Dongxiao" <dongxiao.xu@intel.com>
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? 
Here is how I implemented this in our version of those patches:

/* extra field used in struct page */
union page_ext {
	struct {
#if BITS_PER_LONG < 64
#define GROUP_WIDTH (BITS_PER_LONG - CONFIG_XEN_NETDEV_TX_SHIFT)
#define MAX_GROUPS ((1U << GROUP_WIDTH) - 1)
		unsigned int grp:GROUP_WIDTH;
		unsigned int idx:CONFIG_XEN_NETDEV_TX_SHIFT;
#else
#define MAX_GROUPS UINT_MAX
		unsigned int grp, idx;
#endif
	} e;
	void *mapping;
};

static inline void netif_set_page_ext(struct page *pg, unsigned int group,
				      unsigned int idx)
{
	union page_ext ext = { .e = { .grp = group + 1, .idx = idx } };

	BUILD_BUG_ON(sizeof(ext) > sizeof(ext.mapping));
	pg->mapping = ext.mapping;
}

static inline unsigned int netif_page_group(const struct page *pg)
{
	union page_ext ext = { .mapping = pg->mapping };

	return ext.e.grp - 1;
}

static inline unsigned int netif_page_index(const struct page *pg)
{
	union page_ext ext = { .mapping = pg->mapping };

	return ext.e.idx;
}

CONFIG_XEN_NETDEV_TX_SHIFT is something an earlier patch of
ours introduces (range limited to 5...16; certainly pv-ops could
benefit from having such too), so you will need to replace this
with a literal constant 8 for the time being.

Jan


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