[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH net-next v2 2/2] xen-netback: Grant copy the header instead of map and memcpy
On 01/04/14 10:40, Paul Durrant wrote: I rather prefer to group related operations on the same variable to stay close to each other.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. @@ -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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |