From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: RE: [PATCH net-next v1] xen-netback: make copy batch size configurable Date: Mon, 13 Nov 2017 16:39:09 +0000 Message-ID: <14e335a545264ea49076a69abf461e49@AMSPEX02CL03.citrite.net> References: <20171110193458.14204-1-joao.m.martins@oracle.com> <8c18502b-11ff-be33-a584-a8bdf8960292@oracle.com> <40fc53458a524c64af50b48e43bfd251@AMSPEX02CL03.citrite.net> <20171113163401.eia4pdyimysfg4h6@paddy> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: "netdev@vger.kernel.org" , Wei Liu , "xen-devel@lists.xenproject.org" To: 'Joao Martins' Return-path: Received: from smtp.citrix.com.au ([103.14.252.240]:55935 "EHLO SMTP.CITRIX.COM.AU" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753664AbdKMQjO (ORCPT ); Mon, 13 Nov 2017 11:39:14 -0500 In-Reply-To: <20171113163401.eia4pdyimysfg4h6@paddy> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: > -----Original Message----- > From: Joao Martins [mailto:joao.m.martins@oracle.com] > Sent: 13 November 2017 16:34 > To: Paul Durrant > Cc: netdev@vger.kernel.org; Wei Liu ; 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: > > 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)? Paul