Re: [PATCH net-next v2 2/2] xen-netback: Grant copy the header instead of map and memcpy

[Zoltan Kiss <zoltan.kiss@citrix.com>]

On 01/04/14 10:40, Paul Durrant wrote:
>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
>> netback/netback.c
>> index e781366..ba11ff5 100644
>> --- a/drivers/net/xen-netback/netback.c
>> +++ b/drivers/net/xen-netback/netback.c
>> @@ -915,35 +915,30 @@ static inline void xenvif_grant_handle_reset(struct
>> xenvif *vif,
>>
>>   static int xenvif_tx_check_gop(struct xenvif *vif,
>>   			       struct sk_buff *skb,
>> -			       struct gnttab_map_grant_ref **gopp_map)
>> +			       struct gnttab_map_grant_ref **gopp_map,
>> +			       struct gnttab_copy **gopp_copy)
>>   {
>>   	struct gnttab_map_grant_ref *gop_map = *gopp_map;
>>   	u16 pending_idx = XENVIF_TX_CB(skb)->pending_idx;
>>   	struct skb_shared_info *shinfo = skb_shinfo(skb);
>> -	struct pending_tx_info *tx_info;
>>   	int nr_frags = shinfo->nr_frags;
>> -	int i, err, start;
>> +	int i, err;
>>   	struct sk_buff *first_skb = NULL;
>>
>>   	/* Check status of header. */
>> -	err = gop_map->status;
>> +	err = (*gopp_copy)->status;
>> +	(*gopp_copy)++;
>
> I guess there's no undo work in the case of a copy op so you can bump the copy count here, but it might have been nicer to pair it with the update to the map count.
I rather prefer to group related operations on the same variable to stay 
close to each other.


>> @@ -1067,9 +1051,9 @@ static int xenvif_get_extras(struct xenvif *vif,
>>
>>   		memcpy(&extra, RING_GET_REQUEST(&vif->tx, cons),
>>   		       sizeof(extra));
>> +		vif->tx.req_cons = ++cons;
>>   		if (unlikely(!extra.type ||
>>   			     extra.type >= XEN_NETIF_EXTRA_TYPE_MAX)) {
>> -			vif->tx.req_cons = ++cons;
>>   			netdev_err(vif->dev,
>>   				   "Invalid extra type: %d\n", extra.type);
>>   			xenvif_fatal_tx_err(vif);
>> @@ -1077,7 +1061,6 @@ static int xenvif_get_extras(struct xenvif *vif,
>>   		}
>>
>>   		memcpy(&extras[extra.type - 1], &extra, sizeof(extra));
>> -		vif->tx.req_cons = ++cons;
>
> I know you called this out as unrelated, but I still think it would be better in a separate patch.
Ok


>> @@ -1269,24 +1255,39 @@ static unsigned xenvif_tx_build_gops(struct
>> xenvif *vif, int budget)
>>   			}
>>   		}
>>
>> -		xenvif_tx_create_gop(vif, pending_idx, &txreq, gop);
>> -
>> -		gop++;
>> -
>>   		XENVIF_TX_CB(skb)->pending_idx = pending_idx;
>>
>>   		__skb_put(skb, data_len);
>> +		vif->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
>> +		vif->tx_copy_ops[*copy_ops].source.domid = vif->domid;
>> +		vif->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
>> +
>> +		vif->tx_copy_ops[*copy_ops].dest.u.gmfn =
>> +			virt_to_mfn(skb->data);
>> +		vif->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
>> +		vif->tx_copy_ops[*copy_ops].dest.offset =
>> +			offset_in_page(skb->data);
>> +
>> +		vif->tx_copy_ops[*copy_ops].len = data_len;
>> +		vif->tx_copy_ops[*copy_ops].flags =
>> GNTCOPY_source_gref;
>> +
>> +		(*copy_ops)++;
>>
>>   		skb_shinfo(skb)->nr_frags = ret;
>>   		if (data_len < txreq.size) {
>>   			skb_shinfo(skb)->nr_frags++;
>>   			frag_set_pending_idx(&skb_shinfo(skb)->frags[0],
>>   					     pending_idx);
>> +			xenvif_tx_create_gop(vif, pending_idx, &txreq,
>> gop);
>> +			gop++;
>>   		} else {
>>   			frag_set_pending_idx(&skb_shinfo(skb)->frags[0],
>>   					     INVALID_PENDING_IDX);
>> +			memcpy(&vif->pending_tx_info[pending_idx].req,
>> &txreq,
>> +			       sizeof(txreq));
>>   		}
>>
>> +
>
> Unnecessary whitespace change.
Ok

>
>>   		vif->pending_cons++;
>>
>>   		request_gop = xenvif_get_requests(vif, skb, txfrags, gop);
>> @@ -1301,11 +1302,13 @@ static unsigned xenvif_tx_build_gops(struct
>> xenvif *vif, int budget)
>>
>>   		vif->tx.req_cons = idx;
>>
>> -		if ((gop-vif->tx_map_ops) >= ARRAY_SIZE(vif-
>>> tx_map_ops))
>> +		if (((gop-vif->tx_map_ops) >= ARRAY_SIZE(vif-
>>> tx_map_ops)) ||
>> +		    (*copy_ops >= ARRAY_SIZE(vif->tx_map_ops)))
>
> Do you mean ARRAY_SIZE(vif->tx_copy_ops) here?
Yes, I'll correct it.

>
>>   			break;
>>   	}
>>
>> -	return gop - vif->tx_map_ops;
>> +	(*map_ops) = gop - vif->tx_map_ops;
>> +	return;
>>   }
>>
>>   /* Consolidate skb with a frag_list into a brand new one with local pages on
>> @@ -1377,6 +1380,7 @@ static int xenvif_handle_frag_list(struct xenvif *vif,
>> struct sk_buff *skb)
>>   static int xenvif_tx_submit(struct xenvif *vif)
>>   {
>>   	struct gnttab_map_grant_ref *gop_map = vif->tx_map_ops;
>> +	struct gnttab_copy *gop_copy = vif->tx_copy_ops;
>>   	struct sk_buff *skb;
>>   	int work_done = 0;
>>
>> @@ -1389,7 +1393,7 @@ static int xenvif_tx_submit(struct xenvif *vif)
>>   		txp = &vif->pending_tx_info[pending_idx].req;
>>
>>   		/* Check the remap error code. */
>> -		if (unlikely(xenvif_tx_check_gop(vif, skb, &gop_map))) {
>> +		if (unlikely(xenvif_tx_check_gop(vif, skb, &gop_map,
>> &gop_copy))) {
>>   			netdev_dbg(vif->dev, "netback grant failed.\n");
>
> It could have been the copy that failed. You should probably change the error message.
I've changed it to "netback grant op failed.\n"

>> @@ -1588,22 +1588,26 @@ static inline void xenvif_tx_dealloc_action(struct
>> xenvif *vif)
>>   /* Called after netfront has transmitted */
>>   int xenvif_tx_action(struct xenvif *vif, int budget)
>>   {
>> -	unsigned nr_mops;
>> +	unsigned nr_mops, nr_cops = 0;
>>   	int work_done, ret;
>>
>>   	if (unlikely(!tx_work_todo(vif)))
>>   		return 0;
>>
>> -	nr_mops = xenvif_tx_build_gops(vif, budget);
>> +	xenvif_tx_build_gops(vif, budget, &nr_cops, &nr_mops);
>>
>> -	if (nr_mops == 0)
>> +	if (nr_cops == 0)
>>   		return 0;
>> -
>> -	ret = gnttab_map_refs(vif->tx_map_ops,
>> -			      NULL,
>> -			      vif->pages_to_map,
>> -			      nr_mops);
>> -	BUG_ON(ret);
>> +	else {
>
> Do you need an 'else' here? Unless I'm misreading the diff your previous clause returns.
Indeed, it's not necessary there