On Mon, Nov 13, 2017 at 04:39:09PM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Joao Martins [mailto:joao.m.mart...@oracle.com] > > Sent: 13 November 2017 16:34 > > To: Paul Durrant <paul.durr...@citrix.com> > > Cc: net...@vger.kernel.org; Wei Liu <wei.l...@citrix.com>; xen- > > de...@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: > > > On Mon, Nov 13, 2017 at 11:54:00AM +0000, Joao Martins wrote: > > > > On 11/13/2017 10:33 AM, Paul Durrant wrote: > > > > > On 11/10/2017 19:35 PM, Joao Martins wrote: > > > > [snip] > > > > > > >> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
