Re: [PATCH v3 18/20] net/xen-netback: Make it running on 64KB page granularity

[Wei Liu]

On Fri, Aug 07, 2015 at 05:46:57PM +0100, Julien Grall wrote:
> The PV network protocol is using 4KB page granularity. The goal of this
> patch is to allow a Linux using 64KB page granularity working as a
> network backend on a non-modified Xen.
> 
> It's only necessary to adapt the ring size and break skb data in small
> chunk of 4KB. The rest of the code is relying on the grant table code.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx
> 
[...]
> +#define XEN_NETIF_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, XEN_PAGE_SIZE)
> +#define XEN_NETIF_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, XEN_PAGE_SIZE)
>  
>  struct xenvif_rx_meta {
>  	int id;
> @@ -80,16 +81,18 @@ struct xenvif_rx_meta {
>  /* Discriminate from any valid pending_idx value. */
>  #define INVALID_PENDING_IDX 0xFFFF
>  
> -#define MAX_BUFFER_OFFSET PAGE_SIZE
> +#define MAX_BUFFER_OFFSET XEN_PAGE_SIZE
>  
>  #define MAX_PENDING_REQS XEN_NETIF_TX_RING_SIZE
>  
> +#define MAX_XEN_SKB_FRAGS (65536 / XEN_PAGE_SIZE + 1)
> +

It might be clearer if you add a comment saying the maximum number of
frags is derived from the page size of the grant page, which happens to
be XEN_PAGE_SIZE at the moment. 

In the future we need to figure out the page size of grant page in a
dynamic way. We shall cross the bridge when we get there.

>  /* It's possible for an skb to have a maximal number of frags
>   * but still be less than MAX_BUFFER_OFFSET in size. Thus the
> - * worst-case number of copy operations is MAX_SKB_FRAGS per
> + * worst-case number of copy operations is MAX_XEN_SKB_FRAGS per
>   * ring slot.
>   */
> -#define MAX_GRANT_COPY_OPS (MAX_SKB_FRAGS * XEN_NETIF_RX_RING_SIZE)
> +#define MAX_GRANT_COPY_OPS (MAX_XEN_SKB_FRAGS * XEN_NETIF_RX_RING_SIZE)
>  
>  #define NETBACK_INVALID_HANDLE -1
>  
> @@ -203,7 +206,7 @@ struct xenvif_queue { /* Per-queue data for xenvif */
>  /* Maximum number of Rx slots a to-guest packet may use, including the
>   * slot needed for GSO meta-data.
>   */
> -#define XEN_NETBK_RX_SLOTS_MAX (MAX_SKB_FRAGS + 1)
> +#define XEN_NETBK_RX_SLOTS_MAX ((MAX_XEN_SKB_FRAGS + 1))
>  
>  enum state_bit_shift {
>  	/* This bit marks that the vif is connected */
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 66f1780..c32a9f2 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -263,6 +263,80 @@ static struct xenvif_rx_meta *get_next_rx_buffer(struct xenvif_queue *queue,
>  	return meta;
>  }
>  
[...]
>   * Set up the grant operations for this fragment. If it's a flipping
>   * interface, we also set up the unmap request from here.
> @@ -272,83 +346,52 @@ static void xenvif_gop_frag_copy(struct xenvif_queue *queue, struct sk_buff *skb
>  				 struct page *page, unsigned long size,
>  				 unsigned long offset, int *head)
>  {
> -	struct gnttab_copy *copy_gop;
> -	struct xenvif_rx_meta *meta;
> +	struct gop_frag_copy info = {
> +		.queue = queue,
> +		.npo = npo,
> +		.head = *head,
> +		.gso_type = XEN_NETIF_GSO_TYPE_NONE,
> +	};
>  	unsigned long bytes;
> -	int gso_type = XEN_NETIF_GSO_TYPE_NONE;
>  
>  	if (skb_is_gso(skb)) {
>  		if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4)
> -			gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
> +			info.gso_type = XEN_NETIF_GSO_TYPE_TCPV4;
>  		else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
> -			gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
> +			info.gso_type = XEN_NETIF_GSO_TYPE_TCPV6;
>  	}
>  
>  	/* Data must not cross a page boundary. */
>  	BUG_ON(size + offset > PAGE_SIZE<<compound_order(page));
>  
> -	meta = npo->meta + npo->meta_prod - 1;
> +	info.meta = npo->meta + npo->meta_prod - 1;
>  
>  	/* Skip unused frames from start of page */
>  	page += offset >> PAGE_SHIFT;
>  	offset &= ~PAGE_MASK;
>  
>  	while (size > 0) {
> -		struct xen_page_foreign *foreign;
> -
>  		BUG_ON(offset >= PAGE_SIZE);
> -		BUG_ON(npo->copy_off > MAX_BUFFER_OFFSET);
> -
> -		if (npo->copy_off == MAX_BUFFER_OFFSET)
> -			meta = get_next_rx_buffer(queue, npo);
>  
>  		bytes = PAGE_SIZE - offset;
>  		if (bytes > size)
>  			bytes = size;
>  
> -		if (npo->copy_off + bytes > MAX_BUFFER_OFFSET)
> -			bytes = MAX_BUFFER_OFFSET - npo->copy_off;
> -
> -		copy_gop = npo->copy + npo->copy_prod++;
> -		copy_gop->flags = GNTCOPY_dest_gref;
> -		copy_gop->len = bytes;
> -
> -		foreign = xen_page_foreign(page);
> -		if (foreign) {
> -			copy_gop->source.domid = foreign->domid;
> -			copy_gop->source.u.ref = foreign->gref;
> -			copy_gop->flags |= GNTCOPY_source_gref;
> -		} else {
> -			copy_gop->source.domid = DOMID_SELF;
> -			copy_gop->source.u.gmfn =
> -				virt_to_gfn(page_address(page));
> -		}
> -		copy_gop->source.offset = offset;
> -
> -		copy_gop->dest.domid = queue->vif->domid;
> -		copy_gop->dest.offset = npo->copy_off;
> -		copy_gop->dest.u.ref = npo->copy_gref;
> -
> -		npo->copy_off += bytes;
> -		meta->size += bytes;
> -
> -		offset += bytes;
> +		info.page = page;
> +		gnttab_foreach_grant_in_range(page, offset, bytes,
> +					      xenvif_gop_frag_copy_grant,
> +					      &info);

Looks like I need to at least wait until the API is settle before giving
my ack.

>  		size -= bytes;
> +		offset = 0;

This looks wrong. Should be offset += bytes.

>  
> -		/* Next frame */
> -		if (offset == PAGE_SIZE && size) {
> +		/* Next page */
> +		if (size) {
>  			BUG_ON(!PageCompound(page));
>  			page++;
> -			offset = 0;

And this should not be deleted, I think.

What is the reason for changing offset calculation? I think there is
still compound page when using 64K page.

>  		}
> -
> -		/* Leave a gap for the GSO descriptor. */
> -		if (*head && ((1 << gso_type) & queue->vif->gso_mask))
> -			queue->rx.req_cons++;
> -
> -		*head = 0; /* There must be something in this buffer now. */
> -
>  	}
> +
> +	*head = info.head;
>  }
>  

The reset looks OK.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/