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

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