Re: [PATCH 2/3] xen-netback: switch to per-cpu scratch space

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, May 27, 2013 at 12:29:42PM +0100, Wei Liu wrote:
> There are maximum nr_onlie_cpus netback threads running. We can make use
> of per-cpu scratch space to reduce the size of buffer space when we move
> to 1:1 model.
> 
> In the unlikely event when per-cpu scratch space is not available,
> processing routines will refuse to run on that CPU.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  drivers/net/xen-netback/netback.c |  247 ++++++++++++++++++++++++++++++-------
>  1 file changed, 204 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 54853be..0f69eda 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -37,6 +37,7 @@
>  #include <linux/kthread.h>
>  #include <linux/if_vlan.h>
>  #include <linux/udp.h>
> +#include <linux/cpu.h>
>  
>  #include <net/tcp.h>
>  
> @@ -95,6 +96,24 @@ struct netbk_rx_meta {
>  
>  #define MAX_BUFFER_OFFSET PAGE_SIZE
>  
> +/* Coalescing tx requests before copying makes number of grant
> + * copy ops greater or equal to number of slots required. In
> + * worst case a tx request consumes 2 gnttab_copy. So the size
> + * of tx_copy_ops array should be 2*MAX_PENDING_REQS.
> + */
> +#define TX_COPY_OPS_SIZE (2*MAX_PENDING_REQS)
> +DEFINE_PER_CPU(struct gnttab_copy *, tx_copy_ops);

static
> +
> +/* Given MAX_BUFFER_OFFSET of 4096 the worst case is that each
> + * head/fragment page uses 2 copy operations because it
> + * straddles two buffers in the frontend. So the size of following
> + * arrays should be 2*XEN_NETIF_RX_RING_SIZE.
> + */
> +#define GRANT_COPY_OP_SIZE (2*XEN_NETIF_RX_RING_SIZE)
> +#define META_SIZE (2*XEN_NETIF_RX_RING_SIZE)
> +DEFINE_PER_CPU(struct gnttab_copy *, grant_copy_op);
> +DEFINE_PER_CPU(struct netbk_rx_meta *, meta);

static for both of them.

> +
>  struct xen_netbk {
>  	wait_queue_head_t wq;
>  	struct task_struct *task;
> @@ -116,21 +135,7 @@ struct xen_netbk {
>  	atomic_t netfront_count;
>  
>  	struct pending_tx_info pending_tx_info[MAX_PENDING_REQS];
> -	/* Coalescing tx requests before copying makes number of grant
> -	 * copy ops greater or equal to number of slots required. In
> -	 * worst case a tx request consumes 2 gnttab_copy.
> -	 */
> -	struct gnttab_copy tx_copy_ops[2*MAX_PENDING_REQS];
> -
>  	u16 pending_ring[MAX_PENDING_REQS];
> -
> -	/*
> -	 * Given MAX_BUFFER_OFFSET of 4096 the worst case is that each
> -	 * head/fragment page uses 2 copy operations because it
> -	 * straddles two buffers in the frontend.
> -	 */
> -	struct gnttab_copy grant_copy_op[2*XEN_NETIF_RX_RING_SIZE];
> -	struct netbk_rx_meta meta[2*XEN_NETIF_RX_RING_SIZE];
>  };
>  
>  static struct xen_netbk *xen_netbk;
> @@ -608,12 +613,31 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  	int count;
>  	unsigned long offset;
>  	struct skb_cb_overlay *sco;
> +	struct gnttab_copy *gco = get_cpu_var(grant_copy_op);
> +	struct netbk_rx_meta *m = get_cpu_var(meta);
> +	static int unusable_count;
>  
>  	struct netrx_pending_operations npo = {
> -		.copy  = netbk->grant_copy_op,
> -		.meta  = netbk->meta,
> +		.copy = gco,
> +		.meta = m,
>  	};
>  
> +	if (gco == NULL || m == NULL) {
> +		put_cpu_var(grant_copy_op);
> +		put_cpu_var(meta);
> +		if (unusable_count == 1000) {

printk_ratelimited ?

> +			printk(KERN_ALERT
> +			       "xen-netback: "
> +			       "CPU %d scratch space is not available,"
> +			       " not doing any TX work for netback/%d\n",
> +			       smp_processor_id(),
> +			       (int)(netbk - xen_netbk));

So ... are you going to retry it? Drop it? Can you include in the message the
the mechanism by which you are going to recover?

> +			unusable_count = 0;
> +		} else
> +			unusable_count++;
> +		return;
> +	}
> +
>  	skb_queue_head_init(&rxq);
>  
>  	count = 0;
> @@ -635,27 +659,30 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  			break;
>  	}
>  
> -	BUG_ON(npo.meta_prod > ARRAY_SIZE(netbk->meta));
> +	BUG_ON(npo.meta_prod > META_SIZE);
>  
> -	if (!npo.copy_prod)
> +	if (!npo.copy_prod) {
> +		put_cpu_var(grant_copy_op);
> +		put_cpu_var(meta);
>  		return;
> +	}
>  
> -	BUG_ON(npo.copy_prod > ARRAY_SIZE(netbk->grant_copy_op));
> -	gnttab_batch_copy(netbk->grant_copy_op, npo.copy_prod);
> +	BUG_ON(npo.copy_prod > GRANT_COPY_OP_SIZE);
> +	gnttab_batch_copy(gco, npo.copy_prod);
>  
>  	while ((skb = __skb_dequeue(&rxq)) != NULL) {
>  		sco = (struct skb_cb_overlay *)skb->cb;
>  
>  		vif = netdev_priv(skb->dev);
>  
> -		if (netbk->meta[npo.meta_cons].gso_size && vif->gso_prefix) {
> +		if (m[npo.meta_cons].gso_size && vif->gso_prefix) {
>  			resp = RING_GET_RESPONSE(&vif->rx,
>  						vif->rx.rsp_prod_pvt++);
>  
>  			resp->flags = XEN_NETRXF_gso_prefix | XEN_NETRXF_more_data;
>  
> -			resp->offset = netbk->meta[npo.meta_cons].gso_size;
> -			resp->id = netbk->meta[npo.meta_cons].id;
> +			resp->offset = m[npo.meta_cons].gso_size;
> +			resp->id = m[npo.meta_cons].id;
>  			resp->status = sco->meta_slots_used;
>  
>  			npo.meta_cons++;
> @@ -680,12 +707,12 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  			flags |= XEN_NETRXF_data_validated;
>  
>  		offset = 0;
> -		resp = make_rx_response(vif, netbk->meta[npo.meta_cons].id,
> +		resp = make_rx_response(vif, m[npo.meta_cons].id,
>  					status, offset,
> -					netbk->meta[npo.meta_cons].size,
> +					m[npo.meta_cons].size,
>  					flags);
>  
> -		if (netbk->meta[npo.meta_cons].gso_size && !vif->gso_prefix) {
> +		if (m[npo.meta_cons].gso_size && !vif->gso_prefix) {
>  			struct xen_netif_extra_info *gso =
>  				(struct xen_netif_extra_info *)
>  				RING_GET_RESPONSE(&vif->rx,
> @@ -693,7 +720,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  
>  			resp->flags |= XEN_NETRXF_extra_info;
>  
> -			gso->u.gso.size = netbk->meta[npo.meta_cons].gso_size;
> +			gso->u.gso.size = m[npo.meta_cons].gso_size;
>  			gso->u.gso.type = XEN_NETIF_GSO_TYPE_TCPV4;
>  			gso->u.gso.pad = 0;
>  			gso->u.gso.features = 0;
> @@ -703,7 +730,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  		}
>  
>  		netbk_add_frag_responses(vif, status,
> -					 netbk->meta + npo.meta_cons + 1,
> +					 m + npo.meta_cons + 1,
>  					 sco->meta_slots_used);
>  
>  		RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&vif->rx, ret);
> @@ -726,6 +753,9 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  	if (!skb_queue_empty(&netbk->rx_queue) &&
>  			!timer_pending(&netbk->net_timer))
>  		xen_netbk_kick_thread(netbk);
> +
> +	put_cpu_var(grant_copy_op);
> +	put_cpu_var(meta);
>  }
>  
>  void xen_netbk_queue_tx_skb(struct xenvif *vif, struct sk_buff *skb)
> @@ -1351,9 +1381,10 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
>  	return false;
>  }
>  
> -static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
> +static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk,
> +					struct gnttab_copy *tco)
>  {
> -	struct gnttab_copy *gop = netbk->tx_copy_ops, *request_gop;
> +	struct gnttab_copy *gop = tco, *request_gop;
>  	struct sk_buff *skb;
>  	int ret;
>  
> @@ -1531,16 +1562,17 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  		vif->tx.req_cons = idx;
>  		xen_netbk_check_rx_xenvif(vif);
>  
> -		if ((gop-netbk->tx_copy_ops) >= ARRAY_SIZE(netbk->tx_copy_ops))
> +		if ((gop-tco) >= TX_COPY_OPS_SIZE)
>  			break;
>  	}
>  
> -	return gop - netbk->tx_copy_ops;
> +	return gop - tco;
>  }
>  
> -static void xen_netbk_tx_submit(struct xen_netbk *netbk)
> +static void xen_netbk_tx_submit(struct xen_netbk *netbk,
> +				struct gnttab_copy *tco)
>  {
> -	struct gnttab_copy *gop = netbk->tx_copy_ops;
> +	struct gnttab_copy *gop = tco;
>  	struct sk_buff *skb;
>  
>  	while ((skb = __skb_dequeue(&netbk->tx_queue)) != NULL) {
> @@ -1615,15 +1647,37 @@ static void xen_netbk_tx_submit(struct xen_netbk *netbk)
>  static void xen_netbk_tx_action(struct xen_netbk *netbk)
>  {
>  	unsigned nr_gops;
> +	struct gnttab_copy *tco;
> +	static int unusable_count;
> +
> +	tco = get_cpu_var(tx_copy_ops);
> +
> +	if (tco == NULL) {
> +		put_cpu_var(tx_copy_ops);
> +		if (unusable_count == 1000) {
> +			printk(KERN_ALERT

Ditto. printk_ratelimited.

> +			       "xen-netback: "
> +			       "CPU %d scratch space is not available,"
> +			       " not doing any RX work for netback/%d\n",
> +			       smp_processor_id(),
> +			       (int)(netbk - xen_netbk));

And can you explain what the recovery mechanism is?

> +		} else
> +			unusable_count++;
> +		return;
> +	}
>  
> -	nr_gops = xen_netbk_tx_build_gops(netbk);
> +	nr_gops = xen_netbk_tx_build_gops(netbk, tco);
>  
> -	if (nr_gops == 0)
> +	if (nr_gops == 0) {
> +		put_cpu_var(tx_copy_ops);
>  		return;
> +	}
> +
> +	gnttab_batch_copy(tco, nr_gops);
>  
> -	gnttab_batch_copy(netbk->tx_copy_ops, nr_gops);
> +	xen_netbk_tx_submit(netbk, tco);
>  
> -	xen_netbk_tx_submit(netbk);
> +	put_cpu_var(tx_copy_ops);
>  }
>  
>  static void xen_netbk_idx_release(struct xen_netbk *netbk, u16 pending_idx,
> @@ -1760,6 +1814,93 @@ static int xen_netbk_kthread(void *data)
>  	return 0;
>  }
>  
> +static int __create_percpu_scratch_space(unsigned int cpu)
> +{
> +	if (per_cpu(tx_copy_ops, cpu) ||
> +	    per_cpu(grant_copy_op, cpu) ||
> +	    per_cpu(meta, cpu))
> +		return 0;
> +
> +	per_cpu(tx_copy_ops, cpu) =
> +		vzalloc_node(sizeof(struct gnttab_copy) * TX_COPY_OPS_SIZE,
> +			     cpu_to_node(cpu));
> +
> +	per_cpu(grant_copy_op, cpu) =
> +		vzalloc_node(sizeof(struct gnttab_copy) * GRANT_COPY_OP_SIZE,
> +			     cpu_to_node(cpu));
> +
> +	per_cpu(meta, cpu) =
> +		vzalloc_node(sizeof(struct netbk_rx_meta) * META_SIZE,
> +			     cpu_to_node(cpu));
> +
> +	if (!per_cpu(tx_copy_ops, cpu) ||
> +	    !per_cpu(grant_copy_op, cpu) ||
> +	    !per_cpu(meta, cpu))
> +		return -ENOMEM;

And no freeing? Ah you require the __free_percpu_scratch_space to
do the job for you. Um, why not do it here instead of depending
on the calleer to clean up the mess?

Say:
		{
			__free_percpu_scratch_space(cpu);
			return -ENOMEM;
		}
?


> +
> +	return 0;
> +}
> +
> +static void __free_percpu_scratch_space(unsigned( int cpu)
> +{
> +	void *tmp;
> +
> +	tmp = per_cpu(tx_copy_ops, cpu);
> +	per_cpu(tx_copy_ops, cpu) = NULL;
> +	vfree(tmp);
> +
> +	tmp = per_cpu(grant_copy_op, cpu);
> +	per_cpu(grant_copy_op, cpu) = NULL;
> +	vfree(tmp);
> +
> +	tmp = per_cpu(meta, cpu);
> +	per_cpu(meta, cpu) = NULL;
> +	vfree(tmp);
> +}
> +
> +static int __netback_percpu_callback(struct notifier_block *nfb,
> +				     unsigned long action, void *hcpu)
> +{
> +	unsigned int cpu = (unsigned long)hcpu;
> +	int rc = NOTIFY_DONE;
> +
> +	switch (action) {
> +	case CPU_ONLINE:
> +	case CPU_ONLINE_FROZEN:
> +		printk(KERN_INFO "xen-netback: CPU %d online, creating scratch space\n",
> +		       cpu);

I think this is more of pr_debug type.

> +		rc = __create_percpu_scratch_space(cpu);
> +		if (rc) {
> +			printk(KERN_ALERT "xen-netback: failed to create scratch space for CPU %d\n",
> +			       cpu);
> +			/* There is really nothing more we can do. Free any
> +			 * partially allocated scratch space. When processing
> +			 * routines get to run they will just print warning
> +			 * message and stop processing.
> +			 */
> +			__free_percpu_scratch_space(cpu);

Ugh. Could the code skip creating a kthread on a CPU for which
the per_cpu(meta, cpu) == NULL?

> +			rc = NOTIFY_BAD;
> +		} else
> +			rc = NOTIFY_OK;
> +		break;
> +	case CPU_DEAD:
> +	case CPU_DEAD_FROZEN:
> +		printk(KERN_INFO "xen-netback: CPU %d offline, destroying scratch space\n",
> +		       cpu);
> +		__free_percpu_scratch_space(cpu);
> +		rc = NOTIFY_OK;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return rc;
> +}
> +
> +static struct notifier_block netback_notifier_block = {
> +	.notifier_call = __netback_percpu_callback,
> +};
> +
>  void xen_netbk_unmap_frontend_rings(struct xenvif *vif)
>  {
>  	if (vif->tx.sring)
> @@ -1810,6 +1951,7 @@ static int __init netback_init(void)
>  	int i;
>  	int rc = 0;
>  	int group;
> +	int cpu;
>  
>  	if (!xen_domain())
>  		return -ENODEV;
> @@ -1821,10 +1963,21 @@ static int __init netback_init(void)
>  		fatal_skb_slots = XEN_NETBK_LEGACY_SLOTS_MAX;
>  	}
>  
> +	for_each_online_cpu(cpu) {
> +		rc = __create_percpu_scratch_space(cpu);
> +		if (rc) {
> +			rc = -ENOMEM;
> +			goto failed_init;
> +		}
> +	}
> +	register_hotcpu_notifier(&netback_notifier_block);
> +
>  	xen_netbk_group_nr = num_online_cpus();
>  	xen_netbk = vzalloc(sizeof(struct xen_netbk) * xen_netbk_group_nr);
> -	if (!xen_netbk)
> -		return -ENOMEM;
> +	if (!xen_netbk) {
> +		goto failed_init;
> +		rc = -ENOMEM;
> +	}
>  
>  	for (group = 0; group < xen_netbk_group_nr; group++) {
>  		struct xen_netbk *netbk = &xen_netbk[group];
> @@ -1849,7 +2002,7 @@ static int __init netback_init(void)
>  			printk(KERN_ALERT "kthread_create() fails at netback\n");
>  			del_timer(&netbk->net_timer);
>  			rc = PTR_ERR(netbk->task);
> -			goto failed_init;
> +			goto failed_init_destroy_kthreads;
>  		}
>  
>  		kthread_bind(netbk->task, group);
> @@ -1865,17 +2018,20 @@ static int __init netback_init(void)
>  
>  	rc = xenvif_xenbus_init();
>  	if (rc)
> -		goto failed_init;
> +		goto failed_init_destroy_kthreads;
>  
>  	return 0;
>  
> -failed_init:
> +failed_init_destroy_kthreads:
>  	while (--group >= 0) {
>  		struct xen_netbk *netbk = &xen_netbk[group];
>  		del_timer(&netbk->net_timer);
>  		kthread_stop(netbk->task);
>  	}
>  	vfree(xen_netbk);
> +failed_init:
> +	for_each_online_cpu(cpu)
> +		__free_percpu_scratch_space(cpu);
>  	return rc;
>  
>  }
> @@ -1899,6 +2055,11 @@ static void __exit netback_fini(void)
>  	}
>  
>  	vfree(xen_netbk);
> +
> +	unregister_hotcpu_notifier(&netback_notifier_block);
> +
> +	for_each_online_cpu(i)
> +		__free_percpu_scratch_space(i);
>  }
>  module_exit(netback_fini);
>  
> -- 
> 1.7.10.4
>