Re: [Xen-devel] [RFC PATCH 04/13] xen-netback: implement RX persistent grants

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel