| 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
| ||
|
Message-ID: <20150519153205.GC26335@zion.uk.xensource.com>
Date: Tue, 19 May 2015 16:32:05 +0100
From: Wei Liu <wei.liu2@...rix.com>
To: Joao Martins <joao.martins@...lab.eu>
CC: <xen-devel@...ts.xenproject.org>, <netdev@...r.kernel.org>,
<wei.liu2@...rix.com>, <ian.campbell@...rix.com>,
<david.vrabel@...rix.com>, <boris.ostrovsky@...cle.com>,
<konrad.wilk@...cle.com>
Subject: Re: [RFC PATCH 04/13] xen-netback: implement RX persistent grants
On Tue, May 12, 2015 at 07:18:28PM +0200, Joao Martins wrote:
> It starts by doing a lookup in the tree for a gref. If no persistent
> grant is found on the tree, it will do grant copy and prepare
> the grant maps. Finally valides the grant map and adds it to the tree.
validates?
> After mapped these grants can be pulled from the tree in the subsequent
> requests. If it's out of pages in the tree pool, it will fallback to
> grant copy.
>
Again, this looks complicated. Why use combined scheme? I will do
detailed reviews after we're sure we need such scheme.
> It adds four new fields in the netrx_pending_operations: copy_done
> to track how many copies were made; map_prod and map_cons to track
> how many maps are outstanding validation and finally copy_page for
> the correspondent page (in tree) for copy_gref.
>
> Results are 1.04 Mpps measured with pktgen (pkt_size 64, burst 1)
> with persistent grants versus 1.23 Mpps with grant copy (20%
> regression). With persistent grants it adds up contention on
> queue->wq as the kthread_guest_rx goes to sleep more often. If we
> speed up the sender (burst 2,4 and 8) it goes up to 1.7 Mpps with
> persistent grants. This issue is addressed in later a commit, by
> copying the skb on xenvif_start_xmit() instead of going through
> the RX kthread.
>
> Signed-off-by: Joao Martins <joao.martins@...lab.eu>
> ---
> drivers/net/xen-netback/common.h | 7 ++
> drivers/net/xen-netback/interface.c | 14 ++-
> drivers/net/xen-netback/netback.c | 190 ++++++++++++++++++++++++++++++------
> 3 files changed, 178 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index e5ee220..23deb6a 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -235,6 +235,13 @@ struct xenvif_queue { /* Per-queue data for xenvif */
>
> struct gnttab_copy grant_copy_op[MAX_GRANT_COPY_OPS];
>
> + /* To map the grefs to be added to the tree */
> + struct gnttab_map_grant_ref rx_map_ops[XEN_NETIF_RX_RING_SIZE];
> + struct page *rx_pages_to_map[XEN_NETIF_RX_RING_SIZE];
> + /* Only used if feature-persistent = 1 */
This comment applies to rx_map_ops and rx_pages_to_map as well. Could
you move it up?
> + struct persistent_gnt_tree rx_gnts_tree;
> + struct page *rx_gnts_pages[XEN_NETIF_RX_RING_SIZE];
> +
> /* We create one meta structure per ring request we consume, so
> * the maximum number is the same as the ring size.
> */
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
[...]
>
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 529d7c3..738b6ee 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -413,14 +413,62 @@ static void xenvif_rx_queue_drop_expired(struct xenvif_queue *queue)
> }
>
> struct netrx_pending_operations {
> + unsigned map_prod, map_cons;
> unsigned copy_prod, copy_cons;
> unsigned meta_prod, meta_cons;
> struct gnttab_copy *copy;
> struct xenvif_rx_meta *meta;
> int copy_off;
> grant_ref_t copy_gref;
> + struct page *copy_page;
> + unsigned copy_done;
> };
>
> +static void xenvif_create_rx_map_op(struct xenvif_queue *queue,
> + struct gnttab_map_grant_ref *mop,
> + grant_ref_t ref,
> + struct page *page)
Rename it to xenvif_rx_create_map_op to be consistent with
xenvif_tx_create_map_op?
> +{
> + queue->rx_pages_to_map[mop - queue->rx_map_ops] = page;
> + gnttab_set_map_op(mop,
> + (unsigned long)page_to_kaddr(page),
> + GNTMAP_host_map,
> + ref, queue->vif->domid);
> +}
> +
[...]
> +
> + persistent_gnt = xenvif_pgrant_new(tree, gop_map);
> + if (unlikely(!persistent_gnt)) {
> + netdev_err(queue->vif->dev,
> + "Couldn't add gref to the tree! ref: %d",
> + gop_map->ref);
> + xenvif_page_unmap(queue, gop_map->handle, &page);
> + put_free_pages(tree, &page, 1);
> + kfree(persistent_gnt);
> + persistent_gnt = NULL;
persistent_gnt is already NULL.
So the kfree and = NULL is pointless.
> + continue;
> + }
> +
> + put_persistent_gnt(tree, persistent_gnt);
> + }
> +}
> +
> +/*
> * This is a twin to xenvif_gop_skb. Assume that xenvif_gop_skb was
> * used to set up the operations on the top of
> * netrx_pending_operations, which have since been done. Check that
> * they didn't give any errors and advance over them.
> */
> -static int xenvif_check_gop(struct xenvif *vif, int nr_meta_slots,
> +static int xenvif_check_gop(struct xenvif_queue *queue, int nr_meta_slots,
> struct netrx_pending_operations *npo)
> {
> struct gnttab_copy *copy_op;
> int status = XEN_NETIF_RSP_OKAY;
> int i;
>
> + nr_meta_slots -= npo->copy_done;
> + if (npo->map_prod)
Should be "if (npo->map_prod != npo->map_cons)"?
Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists