Date:   Thu, 21 Dec 2017 17:29:24 +0000
From:   Paul Durrant <Paul.Durrant@...rix.com>
To:     'Joao Martins' <joao.m.martins@...cle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Wei Liu <wei.liu2@...rix.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
Subject: RE: [PATCH net-next v2] xen-netback: make copy batch size
 configurable

> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@...cle.com]
> Sent: 21 December 2017 17:24
> To: netdev@...r.kernel.org
> Cc: Joao Martins <joao.m.martins@...cle.com>; Wei Liu
> <wei.liu2@...rix.com>; Paul Durrant <Paul.Durrant@...rix.com>; xen-
> devel@...ts.xenproject.org
> Subject: [PATCH net-next v2] xen-netback: make copy batch size
> configurable
> 
> Commit eb1723a29b9a ("xen-netback: refactor guest rx") refactored Rx
> handling and as a result decreased max grant copy ops from 4352 to 64.
> Before this commit it would drain the rx_queue (while there are
> enough slots in the ring to put packets) then copy to all pages and write
> responses on the ring. With the refactor we do almost the same albeit
> the last two steps are done every COPY_BATCH_SIZE (64) copies.
> 
> For big packets, the value of 64 means copying 3 packets best case scenario
> (17 copies) and worst-case only 1 packet (34 copies, i.e. if all frags
> plus head cross the 4k grant boundary) which could be the case when
> packets go from local backend process.
> 
> Instead of making it static to 64 grant copies, lets allow the user to
> select its value (while keeping the current as default) by introducing
> the `copy_batch_size` module parameter. This allows users to select
> the higher batches (i.e. for better throughput with big packets) as it
> was prior to the above mentioned commit.
> 
> Signed-off-by: Joao Martins <joao.m.martins@...cle.com>

Reviewed-by: Paul Durrant <paul.durrant@...rix.com>

> ---
> Changes since v1:
>  * move rx_copy.{idx,op} reallocation to separate helper
>  Addressed Paul's comments:
>  * rename xenvif_copy_state#size field to batch_size
>  * argument `size` should be unsigned int
>  * vfree is safe with NULL
>  * realloc rx_copy.{idx,op} after copy op flush
> ---
>  drivers/net/xen-netback/common.h    |  7 +++++--
>  drivers/net/xen-netback/interface.c | 16 +++++++++++++++-
>  drivers/net/xen-netback/netback.c   |  5 +++++
>  drivers/net/xen-netback/rx.c        | 35
> ++++++++++++++++++++++++++++++++++-
>  4 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
> netback/common.h
> index a46a1e94505d..8e4eaf3a507d 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -129,8 +129,9 @@ struct xenvif_stats {
>  #define COPY_BATCH_SIZE 64
> 
>  struct xenvif_copy_state {
> -	struct gnttab_copy op[COPY_BATCH_SIZE];
> -	RING_IDX idx[COPY_BATCH_SIZE];
> +	struct gnttab_copy *op;
> +	RING_IDX *idx;
> +	unsigned int batch_size;
>  	unsigned int num;
>  	struct sk_buff_head *completed;
>  };
> @@ -358,6 +359,7 @@ irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data);
> 
>  void xenvif_rx_action(struct xenvif_queue *queue);
>  void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff
> *skb);
> +int xenvif_rx_copy_realloc(struct xenvif_queue *queue, unsigned int size);
> 
>  void xenvif_carrier_on(struct xenvif *vif);
> 
> @@ -381,6 +383,7 @@ extern unsigned int rx_drain_timeout_msecs;
>  extern unsigned int rx_stall_timeout_msecs;
>  extern unsigned int xenvif_max_queues;
>  extern unsigned int xenvif_hash_cache_size;
> +extern unsigned int xenvif_copy_batch_size;
> 
>  #ifdef CONFIG_DEBUG_FS
>  extern struct dentry *xen_netback_dbg_root;
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-
> netback/interface.c
> index 78ebe494fef0..e12eb64ab0a9 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -518,6 +518,12 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>  {
>  	int err, i;
> 
> +	err = xenvif_rx_copy_realloc(queue, xenvif_copy_batch_size);
> +	if (err) {
> +		netdev_err(queue->vif->dev, "Could not alloc rx_copy\n");
> +		goto err;
> +	}
> +
>  	queue->credit_bytes = queue->remaining_credit = ~0UL;
>  	queue->credit_usec  = 0UL;
>  	timer_setup(&queue->credit_timeout, xenvif_tx_credit_callback, 0);
> @@ -544,7 +550,7 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>  				 queue->mmap_pages);
>  	if (err) {
>  		netdev_err(queue->vif->dev, "Could not reserve
> mmap_pages\n");
> -		return -ENOMEM;
> +		goto err;
>  	}
> 
>  	for (i = 0; i < MAX_PENDING_REQS; i++) {
> @@ -556,6 +562,11 @@ int xenvif_init_queue(struct xenvif_queue *queue)
>  	}
> 
>  	return 0;
> +
> +err:
> +	vfree(queue->rx_copy.op);
> +	vfree(queue->rx_copy.idx);
> +	return -ENOMEM;
>  }
> 
>  void xenvif_carrier_on(struct xenvif *vif)
> @@ -788,6 +799,9 @@ void xenvif_disconnect_ctrl(struct xenvif *vif)
>   */
>  void xenvif_deinit_queue(struct xenvif_queue *queue)
>  {
> +	vfree(queue->rx_copy.op);
> +	vfree(queue->rx_copy.idx);
> +	queue->rx_copy.batch_size = 0;
>  	gnttab_free_pages(MAX_PENDING_REQS, queue->mmap_pages);
>  }
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> netback/netback.c
> index a27daa23c9dc..3a5e1d7ac2f4 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -96,6 +96,11 @@ unsigned int xenvif_hash_cache_size =
> XENVIF_HASH_CACHE_SIZE_DEFAULT;
>  module_param_named(hash_cache_size, xenvif_hash_cache_size, uint,
> 0644);
>  MODULE_PARM_DESC(hash_cache_size, "Number of flows in the hash
> cache");
> 
> +/* This is the maximum batch of grant copies on Rx */
> +unsigned int xenvif_copy_batch_size = COPY_BATCH_SIZE;
> +module_param_named(copy_batch_size, xenvif_copy_batch_size, uint,
> 0644);
> +MODULE_PARM_DESC(copy_batch_size, "Maximum batch of grant copies
> on Rx");
> +
>  static void xenvif_idx_release(struct xenvif_queue *queue, u16
> pending_idx,
>  			       u8 status);
> 
> diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
> index b1cf7c6f407a..07eebd75e668 100644
> --- a/drivers/net/xen-netback/rx.c
> +++ b/drivers/net/xen-netback/rx.c
> @@ -130,6 +130,36 @@ static void xenvif_rx_queue_drop_expired(struct
> xenvif_queue *queue)
>  	}
>  }
> 
> +int xenvif_rx_copy_realloc(struct xenvif_queue *queue, unsigned int size)
> +{
> +	void *op = NULL, *idx = NULL;
> +
> +	if (!size || queue->rx_copy.num)
> +		return -EINVAL;
> +
> +	op = vzalloc(size * sizeof(struct gnttab_copy));
> +	if (!op)
> +		goto err;
> +
> +	idx = vzalloc(size * sizeof(RING_IDX));
> +	if (!idx)
> +		goto err;
> +
> +	vfree(queue->rx_copy.op);
> +	vfree(queue->rx_copy.idx);
> +
> +	queue->rx_copy.op = op;
> +	queue->rx_copy.idx = idx;
> +	queue->rx_copy.batch_size = size;
> +	netdev_dbg(queue->vif->dev, "Reallocated rx_copy for batch size
> %u\n",
> +		   size);
> +	return 0;
> +
> +err:
> +	vfree(op);
> +	return -ENOMEM;
> +}
> +
>  static void xenvif_rx_copy_flush(struct xenvif_queue *queue)
>  {
>  	unsigned int i;
> @@ -162,6 +192,9 @@ static void xenvif_rx_copy_flush(struct xenvif_queue
> *queue)
>  		notify_remote_via_irq(queue->rx_irq);
> 
>  	__skb_queue_purge(queue->rx_copy.completed);
> +
> +	if (unlikely(xenvif_copy_batch_size != queue->rx_copy.batch_size))
> +		xenvif_rx_copy_realloc(queue, xenvif_copy_batch_size);
>  }
> 
>  static void xenvif_rx_copy_add(struct xenvif_queue *queue,
> @@ -172,7 +205,7 @@ static void xenvif_rx_copy_add(struct xenvif_queue
> *queue,
>  	struct page *page;
>  	struct xen_page_foreign *foreign;
> 
> -	if (queue->rx_copy.num == COPY_BATCH_SIZE)
> +	if (queue->rx_copy.num == queue->rx_copy.batch_size)
>  		xenvif_rx_copy_flush(queue);
> 
>  	op = &queue->rx_copy.op[queue->rx_copy.num];
> --
> 2.11.0

Powered by blists - more mailing lists