Messages in this thread | | | Date | Tue, 1 Apr 2014 19:55:35 +0100 | From | Zoltan Kiss <> | Subject | Re: [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: >> 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
| |