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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14e335a545264ea49076a69abf461e49@AMSPEX02CL03.citrite.net>
Date:   Mon, 13 Nov 2017 16:39:09 +0000
From:   Paul Durrant <Paul.Durrant@...rix.com>
To:     'Joao Martins' <joao.m.martins@...cle.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Wei Liu <wei.liu2@...rix.com>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
Subject: RE: [PATCH net-next v1] xen-netback: make copy batch size
 configurable

> -----Original Message-----
> From: Joao Martins [mailto:joao.m.martins@...cle.com]
> Sent: 13 November 2017 16:34
> To: Paul Durrant <Paul.Durrant@...rix.com>
> Cc: netdev@...r.kernel.org; Wei Liu <wei.liu2@...rix.com>; xen-
> devel@...ts.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