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

[Joao Martins <joao.m.martins@oracle.com>]

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 <Paul.Durrant@citrix.com>
> > Cc: netdev@vger.kernel.org; Wei Liu <wei.liu2@citrix.com>; 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