From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [Pv-ops][PATCH 3/4 v2] Netback: Multiple tasklets support Date: Mon, 3 May 2010 12:06:44 -0400 Message-ID: <20100503160644.GE31299@phenom.dumpdata.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Xu, Dongxiao" Cc: Jeremy Fitzhardinge , "xen-devel@lists.xensource.com" , Steven Smith List-Id: xen-devel@lists.xenproject.org On Thu, Apr 29, 2010 at 10:28:50PM +0800, Xu, Dongxiao wrote: > Netback: Multiple tasklets support. > > Now 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 ^^^ -> pair > 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. scripts/checkpatch.pl --strict ~/0003-Netback-Multiple-tasklets-support.patch CHECK: spinlock_t definition without comment #42: FILE: drivers/xen/netback/common.h:292: + spinlock_t group_operation_lock; total: 0 errors, 0 warnings, 1 checks, 626 lines checked /home/konrad/0003-Netback-Multiple-tasklets-support.patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. > > Signed-off-by: Dongxiao Xu +static void netbk_add_netif(struct xen_netbk *netbk, int group_nr, + struct xen_netif *netif) +{ + int i; + int min_netfront_count; + int min_group = 0; + spin_lock(&netbk->group_operation_lock); + min_netfront_count = netbk[0].netfront_count; + for (i = 0; i < group_nr; i++) { + if (netbk[i].netfront_count < min_netfront_count) { + min_group = i; + min_netfront_count = netbk[i].netfront_count; Should you have a 'break' here? I am not sure if it makes sense to go through all of the tasklets to set the min_group and min_netfrount_count to the last one? + } + } + + netif->group = min_group; + netbk[netif->group].netfront_count++; + spin_unlock(&netbk->group_operation_lock); +} + +static void netbk_remove_netif(struct xen_netbk *netbk, struct xen_netif *netif) +{ + spin_lock(&netbk->group_operation_lock); + netbk[netif->group].netfront_count--; + spin_unlock(&netbk->group_operation_lock); +} + static void __netif_up(struct xen_netif *netif) { enable_irq(netif->irq); @@ -333,6 +360,8 @@ int netif_map(struct xen_netif *netif, unsigned long tx_ring_ref, if (netif->rx_comms_area == NULL) goto err_rx; + netbk_add_netif(xen_netbk, xen_netbk_group_nr, netif); + Say you have 7 VIFs and only 4 VCPUs, are these netfront_count values correct? netbk[0].netfront_count == 1; /* vif0 added */ netbk[3].netfront_count == 1; /* vif1 added */ netbk[2].netfront_count == 1; /* vif2 added */ netbk[1].netfront_count == 1; /* vif3 added */ netbk[0].netfront_count == 2; /* vif4 added */ netbk[3].netfront_count == 2; /* vif5 added */ netbk[2].netfront_count == 2; /* vif6 added */ I just want to make sure I understand the allocation algorithm correctly.