From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joao Martins Subject: Re: [PATCH net-next v1] xen-netback: make copy batch size configurable Date: Mon, 13 Nov 2017 16:53:37 +0000 Message-ID: <20171113165336.yhd42fjjxbiymxsf@paddy> References: <20171110193458.14204-1-joao.m.martins@oracle.com> <8c18502b-11ff-be33-a584-a8bdf8960292@oracle.com> <40fc53458a524c64af50b48e43bfd251@AMSPEX02CL03.citrite.net> <20171113163401.eia4pdyimysfg4h6@paddy> <14e335a545264ea49076a69abf461e49@AMSPEX02CL03.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "netdev@vger.kernel.org" , Wei Liu , "xen-devel@lists.xenproject.org" To: Paul Durrant Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:38832 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753640AbdKMQyL (ORCPT ); Mon, 13 Nov 2017 11:54:11 -0500 Content-Disposition: inline In-Reply-To: <14e335a545264ea49076a69abf461e49@AMSPEX02CL03.citrite.net> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Nov 13, 2017 at 04:39:09PM +0000, Paul Durrant wrote: > > -----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)? /facepalm Yes, after the flush should make things much simpler. Joao