[PATCH net-next v1] xen-netback: make copy batch size configurable

Subsystems: networking drivers, the rest, xen network backend driver

STALE3198d

9 messages, 3 authors, 2017-11-13 · open the first message on its own page

[PATCH net-next v1] xen-netback: make copy batch size configurable

From: Joao Martins <hidden>
Date: 2017-11-10 19:35:17

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 <redacted>
---
 drivers/net/xen-netback/common.h    |  6 ++++--
 drivers/net/xen-netback/interface.c | 25 ++++++++++++++++++++++++-
 drivers/net/xen-netback/netback.c   |  5 +++++
 drivers/net/xen-netback/rx.c        |  5 ++++-
 4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a46a1e94505d..a5fe36e098a7 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 size;
 	unsigned int num;
 	struct sk_buff_head *completed;
 };
@@ -381,6 +382,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 d6dff347f896..a558868a883f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -516,7 +516,20 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 
 int xenvif_init_queue(struct xenvif_queue *queue)
 {
+	int size = xenvif_copy_batch_size;
 	int err, i;
+	void *addr;
+
+	addr = vzalloc(size * sizeof(struct gnttab_copy));
+	if (!addr)
+		goto err;
+	queue->rx_copy.op = addr;
+
+	addr = vzalloc(size * sizeof(RING_IDX));
+	if (!addr)
+		goto err;
+	queue->rx_copy.idx = addr;
+	queue->rx_copy.size = size;
 
 	queue->credit_bytes = queue->remaining_credit = ~0UL;
 	queue->credit_usec  = 0UL;
@@ -544,7 +557,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 +569,13 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 	}
 
 	return 0;
+
+err:
+	if (queue->rx_copy.op)
+		vfree(queue->rx_copy.op);
+	if (queue->rx_copy.idx)
+		vfree(queue->rx_copy.idx);
+	return -ENOMEM;
 }
 
 void xenvif_carrier_on(struct xenvif *vif)
@@ -788,6 +808,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.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..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;
 
-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
+
+	if (queue->rx_copy.num == batch_size)
 		xenvif_rx_copy_flush(queue);
 
 	op = &queue->rx_copy.op[queue->rx_copy.num];
-- 
2.11.0

Re: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Paul Durrant <hidden>
Date: 2017-11-13 10:33:38

quoted hunk
-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 10 November 2017 19:35
To: netdev@vger.kernel.org
Cc: Joao Martins <redacted>; Wei Liu
[off-list ref]; Paul Durrant [off-list ref]; xen-
devel@lists.xenproject.org
Subject: [PATCH net-next v1] 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 <redacted>
---
 drivers/net/xen-netback/common.h    |  6 ++++--
 drivers/net/xen-netback/interface.c | 25 ++++++++++++++++++++++++-
 drivers/net/xen-netback/netback.c   |  5 +++++
 drivers/net/xen-netback/rx.c        |  5 ++++-
 4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
netback/common.h
index a46a1e94505d..a5fe36e098a7 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 size;
Could you name this batch_size, or something like that to make it clear what it means?
quoted hunk
 	unsigned int num;
 	struct sk_buff_head *completed;
 };
@@ -381,6 +382,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 d6dff347f896..a558868a883f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -516,7 +516,20 @@ struct xenvif *xenvif_alloc(struct device *parent,
domid_t domid,

 int xenvif_init_queue(struct xenvif_queue *queue)
 {
+	int size = xenvif_copy_batch_size;
unsigned int
 	int err, i;
+	void *addr;
+
+	addr = vzalloc(size * sizeof(struct gnttab_copy));
Does the memory need to be zeroed?
+	if (!addr)
+		goto err;
+	queue->rx_copy.op = addr;
+
+	addr = vzalloc(size * sizeof(RING_IDX));
Likewise.
quoted hunk
+	if (!addr)
+		goto err;
+	queue->rx_copy.idx = addr;
+	queue->rx_copy.size = size;

 	queue->credit_bytes = queue->remaining_credit = ~0UL;
 	queue->credit_usec  = 0UL;
@@ -544,7 +557,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 +569,13 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 	}

 	return 0;
+
+err:
+	if (queue->rx_copy.op)
+		vfree(queue->rx_copy.op);
vfree is safe to be called with NULL.
quoted hunk
+	if (queue->rx_copy.idx)
+		vfree(queue->rx_copy.idx);
+	return -ENOMEM;
 }

 void xenvif_carrier_on(struct xenvif *vif)
@@ -788,6 +808,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.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..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always identical? Why do you need this statement (and hence stack variable)?

  Paul
+
+	if (queue->rx_copy.num == batch_size)
 		xenvif_rx_copy_flush(queue);

 	op = &queue->rx_copy.op[queue->rx_copy.num];
--
2.11.0

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Jan Beulich <hidden>
Date: 2017-11-13 10:50:15

quoted
quoted
On 13.11.17 at 11:33, [off-list ref] wrote:
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 10 November 2017 19:35
--- 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);
Isn't the "owner-write" permission here ...
quoted
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always identical? 
Why do you need this statement (and hence stack variable)?
... the answer to your question?

Jan

RE: [Xen-devel] [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Paul Durrant <hidden>
Date: 2017-11-13 11:03:48

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com]
Sent: 13 November 2017 10:50
To: Paul Durrant <redacted>
Cc: Wei Liu <redacted>; xen-devel@lists.xenproject.org; 'Joao
Martins' [off-list ref]; netdev@vger.kernel.org
Subject: Re: [Xen-devel] [PATCH net-next v1] xen-netback: make copy batch
size configurable
quoted
quoted
quoted
On 13.11.17 at 11:33, [off-list ref] wrote:
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 10 November 2017 19:35
--- 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);
Isn't the "owner-write" permission here ...
quoted
quoted
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always
identical?
quoted
Why do you need this statement (and hence stack variable)?
... the answer to your question?
Yes, I guess it could be... but since there's no re-alloc code for the arrays I wonder whether the intention was to make this dynamic or not.

  Paul
Jan

Re: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Joao Martins <hidden>
Date: 2017-11-13 11:54:17

On 11/13/2017 10:33 AM, Paul Durrant wrote:
quoted
-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 10 November 2017 19:35
To: netdev@vger.kernel.org
Cc: Joao Martins <redacted>; Wei Liu
[off-list ref]; Paul Durrant [off-list ref]; xen-
devel@lists.xenproject.org
Subject: [PATCH net-next v1] 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 <redacted>
---
 drivers/net/xen-netback/common.h    |  6 ++++--
 drivers/net/xen-netback/interface.c | 25 ++++++++++++++++++++++++-
 drivers/net/xen-netback/netback.c   |  5 +++++
 drivers/net/xen-netback/rx.c        |  5 ++++-
 4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
netback/common.h
index a46a1e94505d..a5fe36e098a7 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 size;
Could you name this batch_size, or something like that to make it clear what it means?
Yeap, will change it.
quoted
 	unsigned int num;
 	struct sk_buff_head *completed;
 };
@@ -381,6 +382,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 d6dff347f896..a558868a883f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -516,7 +516,20 @@ struct xenvif *xenvif_alloc(struct device *parent,
domid_t domid,

 int xenvif_init_queue(struct xenvif_queue *queue)
 {
+	int size = xenvif_copy_batch_size;
unsigned int
quoted
quoted
 	int err, i;
+	void *addr;
+
+	addr = vzalloc(size * sizeof(struct gnttab_copy));
Does the memory need to be zeroed?
It doesn't need to be but given that xenvif_queue is zeroed (which included this
region) thus thought I would leave the same way.
quoted
+	if (!addr)
+		goto err;
+	queue->rx_copy.op = addr;
+
+	addr = vzalloc(size * sizeof(RING_IDX));
Likewise.
quoted
+	if (!addr)
+		goto err;
+	queue->rx_copy.idx = addr;
+	queue->rx_copy.size = size;

 	queue->credit_bytes = queue->remaining_credit = ~0UL;
 	queue->credit_usec  = 0UL;
@@ -544,7 +557,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 +569,13 @@ int xenvif_init_queue(struct xenvif_queue *queue)
 	}

 	return 0;
+
+err:
+	if (queue->rx_copy.op)
+		vfree(queue->rx_copy.op);
vfree is safe to be called with NULL.
Oh, almost forgot - thanks.
quoted
+	if (queue->rx_copy.idx)
+		vfree(queue->rx_copy.idx);
+	return -ENOMEM;
 }

 void xenvif_carrier_on(struct xenvif *vif)
@@ -788,6 +808,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.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..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always identical? Why do you need this statement (and hence stack variable)?
This statement was to allow to be changed dynamically and would affect all newly
created guests or running guests if value happened to be smaller than initially
allocated. But I suppose I should make behaviour more consistent with the other
params we have right now and just look at initially allocated one
`queue->rx_copy.batch_size` ?

Joao

RE: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Paul Durrant <hidden>
Date: 2017-11-13 11:58:08

-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 13 November 2017 11:54
To: Paul Durrant <redacted>
Cc: netdev@vger.kernel.org; Wei Liu <redacted>; xen-
devel@lists.xenproject.org
Subject: Re: [PATCH net-next v1] xen-netback: make copy batch size
configurable

On 11/13/2017 10:33 AM, Paul Durrant wrote:
quoted
quoted
-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 10 November 2017 19:35
To: netdev@vger.kernel.org
Cc: Joao Martins <redacted>; Wei Liu
[off-list ref]; Paul Durrant [off-list ref]; xen-
devel@lists.xenproject.org
Subject: [PATCH net-next v1] 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
quoted
quoted
(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 <redacted>
---
 drivers/net/xen-netback/common.h    |  6 ++++--
 drivers/net/xen-netback/interface.c | 25
++++++++++++++++++++++++-
quoted
quoted
 drivers/net/xen-netback/netback.c   |  5 +++++
 drivers/net/xen-netback/rx.c        |  5 ++++-
 4 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-
netback/common.h
index a46a1e94505d..a5fe36e098a7 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 size;
Could you name this batch_size, or something like that to make it clear
what it means?
quoted
Yeap, will change it.
quoted
quoted
 	unsigned int num;
 	struct sk_buff_head *completed;
 };
@@ -381,6 +382,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 d6dff347f896..a558868a883f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -516,7 +516,20 @@ struct xenvif *xenvif_alloc(struct device *parent,
domid_t domid,

 int xenvif_init_queue(struct xenvif_queue *queue)
 {
+	int size = xenvif_copy_batch_size;
unsigned int
quoted
quoted
 	int err, i;
+	void *addr;
+
+	addr = vzalloc(size * sizeof(struct gnttab_copy));
Does the memory need to be zeroed?
It doesn't need to be but given that xenvif_queue is zeroed (which included
this
region) thus thought I would leave the same way.
Ok.
quoted
quoted
+	if (!addr)
+		goto err;
+	queue->rx_copy.op = addr;
+
+	addr = vzalloc(size * sizeof(RING_IDX));
Likewise.
quoted
+	if (!addr)
+		goto err;
+	queue->rx_copy.idx = addr;
+	queue->rx_copy.size = size;

 	queue->credit_bytes = queue->remaining_credit = ~0UL;
 	queue->credit_usec  = 0UL;
@@ -544,7 +557,7 @@ int xenvif_init_queue(struct xenvif_queue
*queue)
quoted
quoted
 				 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 +569,13 @@ int xenvif_init_queue(struct xenvif_queue
*queue)
quoted
quoted
 	}

 	return 0;
+
+err:
+	if (queue->rx_copy.op)
+		vfree(queue->rx_copy.op);
vfree is safe to be called with NULL.
Oh, almost forgot - thanks.
quoted
quoted
+	if (queue->rx_copy.idx)
+		vfree(queue->rx_copy.idx);
+	return -ENOMEM;
 }

 void xenvif_carrier_on(struct xenvif *vif)
@@ -788,6 +808,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.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
quoted
quoted
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..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always
identical? Why do you need this statement (and hence stack variable)?
quoted
This statement was to allow to be changed dynamically and would affect all
newly
created guests or running guests if value happened to be smaller than initially
allocated. But I suppose I should make behaviour more consistent with the
other
params we have right now and just look at initially allocated one
`queue->rx_copy.batch_size` ?
Yes, that would certainly be consistent but I can see value in allowing it to be dynamically tuned, so perhaps adding some re-allocation code to allow the batch to be grown as well as shrunk might be nice.

  Paul
Joao

Re: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Joao Martins <hidden>
Date: 2017-11-13 16:34:20

On Mon, Nov 13, 2017 at 11:58:03AM +0000, Paul Durrant wrote:
On Mon, Nov 13, 2017 at 11:54:00AM +0000, Joao Martins wrote:
quoted
On 11/13/2017 10:33 AM, Paul Durrant wrote:
quoted
On 11/10/2017 19:35 PM, Joao Martins wrote:
[snip]
quoted
quoted
quoted
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index b1cf7c6f407a..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always
identical? Why do you need this statement (and hence stack variable)?
This statement was to allow to be changed dynamically and would
affect all newly created guests or running guests if value happened
to be smaller than initially allocated. But I suppose I should make
behaviour more consistent with the other params we have right now
and just look at initially allocated one `queue->rx_copy.batch_size` ?
Yes, that would certainly be consistent but I can see value in
allowing it to be dynamically tuned, so perhaps adding some re-allocation
code to allow the batch to be grown as well as shrunk might be nice.
The shrink one we potentially risk losing data, so we need to gate the
reallocation whenever `rx_copy.num` is less than the new requested
batch. Worst case means guestrx_thread simply uses the initial
allocated value.

Anyhow, something like the below scissored diff (on top of your comments):
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index a165a4123396..8e4eaf3a507d 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -359,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);
 
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 1892bf9327e4..14613b5fcccb 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -516,20 +516,13 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 
 int xenvif_init_queue(struct xenvif_queue *queue)
 {
-	unsigned int size = xenvif_copy_batch_size;
 	int err, i;
-	void *addr;
-
-	addr = vzalloc(size * sizeof(struct gnttab_copy));
-	if (!addr)
-		goto err;
-	queue->rx_copy.op = addr;
 
-	addr = vzalloc(size * sizeof(RING_IDX));
-	if (!addr)
+	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->rx_copy.idx = addr;
-	queue->rx_copy.batch_size = size;
+	}
 
 	queue->credit_bytes = queue->remaining_credit = ~0UL;
 	queue->credit_usec  = 0UL;
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index be3946cdaaf6..f54bfe72188c 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -130,6 +130,51 @@ 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;
+
+	/* No reallocation if new size doesn't fit ongoing requests */
+	if (!size || queue->rx_copy.num > size)
+		return -EINVAL;
+
+	op = vzalloc(size * sizeof(struct gnttab_copy));
+	if (!op)
+		goto err;
+
+	idx = vzalloc(size * sizeof(RING_IDX));
+	if (!idx)
+		goto err;
+
+	/* Ongoing requests need copying */
+	if (queue->rx_copy.num) {
+		unsigned int tmp;
+
+		tmp = queue->rx_copy.num * sizeof(struct gnttab_copy);
+		memcpy(op, queue->rx_copy.op, tmp);
+
+		tmp = queue->rx_copy.num * sizeof(RING_IDX);
+		memcpy(idx, queue->rx_copy.idx, tmp);
+	}
+
+	if (queue->rx_copy.op || queue->rx_copy.idx) {
+		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);
+	vfree(idx);
+	return -ENOMEM;
+}
+
 static void xenvif_rx_copy_flush(struct xenvif_queue *queue)
 {
 	unsigned int i;
@@ -168,14 +213,14 @@ static void xenvif_rx_copy_add(struct xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
-	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;
 
-	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.batch_size);
+	if (unlikely(xenvif_copy_batch_size != queue->rx_copy.batch_size))
+		xenvif_rx_copy_realloc(queue, xenvif_copy_batch_size);
 
-	if (queue->rx_copy.num == 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];

RE: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Paul Durrant <hidden>
Date: 2017-11-13 16:39:14

-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 13 November 2017 16:34
To: Paul Durrant <redacted>
Cc: netdev@vger.kernel.org; Wei Liu <redacted>; xen-
devel@lists.xenproject.org
Subject: Re: [PATCH net-next v1] xen-netback: make copy batch size
configurable

On Mon, Nov 13, 2017 at 11:58:03AM +0000, Paul Durrant wrote:
quoted
On Mon, Nov 13, 2017 at 11:54:00AM +0000, Joao Martins wrote:
quoted
On 11/13/2017 10:33 AM, Paul Durrant wrote:
quoted
On 11/10/2017 19:35 PM, Joao Martins wrote:
[snip]
quoted
quoted
quoted
quoted
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-
netback/rx.c
quoted
quoted
quoted
quoted
index b1cf7c6f407a..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct
xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue-
rx_copy.size);
quoted
quoted
Surely queue->rx_copy.size and xenvif_copy_batch_size are always
identical? Why do you need this statement (and hence stack variable)?
This statement was to allow to be changed dynamically and would
affect all newly created guests or running guests if value happened
to be smaller than initially allocated. But I suppose I should make
behaviour more consistent with the other params we have right now
and just look at initially allocated one `queue->rx_copy.batch_size` ?
Yes, that would certainly be consistent but I can see value in
allowing it to be dynamically tuned, so perhaps adding some re-allocation
code to allow the batch to be grown as well as shrunk might be nice.
The shrink one we potentially risk losing data, so we need to gate the
reallocation whenever `rx_copy.num` is less than the new requested
batch. Worst case means guestrx_thread simply uses the initial
allocated value.
Can't you just re-alloc immediately after the flush (when num is guaranteed to be zero)?

  Paul

Re: [PATCH net-next v1] xen-netback: make copy batch size configurable

From: Joao Martins <hidden>
Date: 2017-11-13 16:54:11

On Mon, Nov 13, 2017 at 04:39:09PM +0000, Paul Durrant wrote:
quoted
-----Original Message-----
From: Joao Martins [mailto:joao.m.martins@oracle.com]
Sent: 13 November 2017 16:34
To: Paul Durrant <redacted>
Cc: netdev@vger.kernel.org; Wei Liu <redacted>; xen-
devel@lists.xenproject.org
Subject: Re: [PATCH net-next v1] xen-netback: make copy batch size
configurable

On Mon, Nov 13, 2017 at 11:58:03AM +0000, Paul Durrant wrote:
quoted
On Mon, Nov 13, 2017 at 11:54:00AM +0000, Joao Martins wrote:
quoted
On 11/13/2017 10:33 AM, Paul Durrant wrote:
quoted
On 11/10/2017 19:35 PM, Joao Martins wrote:
[snip]
quoted
quoted
quoted
quoted
diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c
index b1cf7c6f407a..793a85f61f9d 100644
--- a/drivers/net/xen-netback/rx.c
+++ b/drivers/net/xen-netback/rx.c
@@ -168,11 +168,14 @@ static void xenvif_rx_copy_add(struct xenvif_queue *queue,
 			       struct xen_netif_rx_request *req,
 			       unsigned int offset, void *data, size_t len)
 {
+	unsigned int batch_size;
 	struct gnttab_copy *op;
 	struct page *page;
 	struct xen_page_foreign *foreign;

-	if (queue->rx_copy.num == COPY_BATCH_SIZE)
+	batch_size = min(xenvif_copy_batch_size, queue->rx_copy.size);
Surely queue->rx_copy.size and xenvif_copy_batch_size are always
identical? Why do you need this statement (and hence stack variable)?
This statement was to allow to be changed dynamically and would
affect all newly created guests or running guests if value happened
to be smaller than initially allocated. But I suppose I should make
behaviour more consistent with the other params we have right now
and just look at initially allocated one `queue->rx_copy.batch_size` ?
Yes, that would certainly be consistent but I can see value in
allowing it to be dynamically tuned, so perhaps adding some re-allocation
code to allow the batch to be grown as well as shrunk might be nice.
The shrink one we potentially risk losing data, so we need to gate the
reallocation whenever `rx_copy.num` is less than the new requested
batch. Worst case means guestrx_thread simply uses the initial
allocated value.
Can't you just re-alloc immediately after the flush (when num is
guaranteed to be zero)?
/facepalm

Yes, after the flush should make things much simpler.

Joao
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help