| Message ID | 20171113163401.eia4pdyimysfg4h6@paddy (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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)? Paul
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
diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h index a165a4123396..8e4eaf3a507d 100644 --- a/drivers/net/xen-netback/common.h +++ b/drivers/net/xen-netback/common.h @@ -359,6 +359,7 @@ irqreturn_t xenvif_ctrl_irq_fn(int irq, void *data); void xenvif_rx_action(struct xenvif_queue *queue); void xenvif_rx_queue_tail(struct xenvif_queue *queue, struct sk_buff *skb); +int xenvif_rx_copy_realloc(struct xenvif_queue *queue, unsigned int size); void xenvif_carrier_on(struct xenvif *vif); diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c index 1892bf9327e4..14613b5fcccb 100644 --- a/drivers/net/xen-netback/interface.c +++ b/drivers/net/xen-netback/interface.c @@ -516,20 +516,13 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid, int xenvif_init_queue(struct xenvif_queue *queue) { - unsigned int size = xenvif_copy_batch_size; int err, i; - void *addr; - - addr = vzalloc(size * sizeof(struct gnttab_copy)); - if (!addr) - goto err; - queue->rx_copy.op = addr; - addr = vzalloc(size * sizeof(RING_IDX)); - if (!addr) + err = xenvif_rx_copy_realloc(queue, xenvif_copy_batch_size); + if (err) { + netdev_err(queue->vif->dev, "Could not alloc rx_copy\n"); goto err; - queue->rx_copy.idx = addr; - queue->rx_copy.batch_size = size; + } queue->credit_bytes = queue->remaining_credit = ~0UL; queue->credit_usec = 0UL; diff --git a/drivers/net/xen-netback/rx.c b/drivers/net/xen-netback/rx.c index be3946cdaaf6..f54bfe72188c 100644 --- a/drivers/net/xen-netback/rx.c +++ b/drivers/net/xen-netback/rx.c @@ -130,6 +130,51 @@ static void xenvif_rx_queue_drop_expired(struct xenvif_queue *queue) } } +int xenvif_rx_copy_realloc(struct xenvif_queue *queue, unsigned int size) +{ + void *op = NULL, *idx = NULL; + + /* No reallocation if new size doesn't fit ongoing requests */ + if (!size || queue->rx_copy.num > size) + return -EINVAL; + + op = vzalloc(size * sizeof(struct gnttab_copy)); + if (!op) + goto err; + + idx = vzalloc(size * sizeof(RING_IDX)); + if (!idx) + goto err; + + /* Ongoing requests need copying */ + if (queue->rx_copy.num) { + unsigned int tmp; + + tmp = queue->rx_copy.num * sizeof(struct gnttab_copy); + memcpy(op, queue->rx_copy.op, tmp); + + tmp = queue->rx_copy.num * sizeof(RING_IDX); + memcpy(idx, queue->rx_copy.idx, tmp); + } + + if (queue->rx_copy.op || queue->rx_copy.idx) { + vfree(queue->rx_copy.op); + vfree(queue->rx_copy.idx); + } + + queue->rx_copy.op = op; + queue->rx_copy.idx = idx; + queue->rx_copy.batch_size = size; + netdev_dbg(queue->vif->dev, "Reallocated rx_copy for batch size %u\n", + size); + return 0; + +err: + vfree(op); + vfree(idx); + return -ENOMEM; +} + static void xenvif_rx_copy_flush(struct xenvif_queue *queue) { unsigned int i; @@ -168,14 +213,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; - batch_size = min(xenvif_copy_batch_size, queue->rx_copy.batch_size); + if (unlikely(xenvif_copy_batch_size != queue->rx_copy.batch_size)) + xenvif_rx_copy_realloc(queue, xenvif_copy_batch_size); - if (queue->rx_copy.num == batch_size) + if (queue->rx_copy.num == queue->rx_copy.batch_size) xenvif_rx_copy_flush(queue); op = &queue->rx_copy.op[queue->rx_copy.num];