Re: [Xen-devel] [PATCH net-next v3 2/9] xen-netback: Change TX path from grant copy to mapping

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Jan 10, 2014 at 11:35:08AM +0000, Zoltan Kiss wrote:
[...]
> 
> >>@@ -920,6 +852,18 @@ static int xenvif_tx_check_gop(struct xenvif *vif,
> >>    err = gop->status;
> >>    if (unlikely(err))
> >>            xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_ERROR);
> >>+   else {
> >>+           if (vif->grant_tx_handle[pending_idx] !=
> >>+                   NETBACK_INVALID_HANDLE) {
> >>+                   netdev_err(vif->dev,
> >>+                           "Stale mapped handle! pending_idx %x handle 
> >>%x\n",
> >>+                           pending_idx, vif->grant_tx_handle[pending_idx]);
> >>+                   BUG();
> >>+           }
> >>+           set_phys_to_machine(idx_to_pfn(vif, pending_idx),
> >>+                   FOREIGN_FRAME(gop->dev_bus_addr >> PAGE_SHIFT));
> >
> >What happens when you don't have this?
> Your frags will be filled with garbage. I don't understand exactly
> what this function does, someone might want to enlighten us? I've
> took it's usage from classic kernel.
> Also, it might be worthwhile to check the return value and BUG if
> it's false, but I don't know what exactly that return value means.
> 

This is actually part of gnttab_map_refs. As you're using hypercall
directly this becomes very fragile.

So the right thing to do is to fix gnttab_map_refs.

> >
> >>            if (skb_is_nonlinear(skb) && skb_headlen(skb) < PKT_PROT_LEN) {
> >>                    int target = min_t(int, skb->len, PKT_PROT_LEN);
> >>@@ -1581,6 +1541,8 @@ static int xenvif_tx_submit(struct xenvif *vif)
> >>            if (checksum_setup(vif, skb)) {
> >>                    netdev_dbg(vif->dev,
> >>                               "Can't setup checksum in net_tx_action\n");
> >>+                   if (skb_shinfo(skb)->destructor_arg)
> >>+                           skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> >
> >Do you still care setting the flag even if this skb is not going to be
> >delivered? If so can you state clearly the reason just like the
> >following hunk?
> Of course, otherwise the pages wouldn't be sent back to the guest.
> I've added a comment.
> 

OK, Thanks! That means whenever SKB leaves netback we need to add this
flag.

> >>@@ -1715,7 +1685,7 @@ static inline void xenvif_tx_dealloc_action(struct 
> >>xenvif *vif)
> >>  int xenvif_tx_action(struct xenvif *vif, int budget)
> >>  {
> >>    unsigned nr_gops;
> >>-   int work_done;
> >>+   int work_done, ret;
> >>
> >>    if (unlikely(!tx_work_todo(vif)))
> >>            return 0;
> >>@@ -1725,7 +1695,10 @@ int xenvif_tx_action(struct xenvif *vif, int budget)
> >>    if (nr_gops == 0)
> >>            return 0;
> >>
> >>-   gnttab_batch_copy(vif->tx_copy_ops, nr_gops);
> >>+   ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
> >>+                   vif->tx_map_ops,
> >>+                   nr_gops);
> >
> >Why do you need to replace gnttab_batch_copy with hypercall? In the
> >ideal situation gnttab_batch_copy should behave the same as directly
> >hypercall but it also handles GNTST_eagain for you.
> 
> I don't need gnttab_batch_copy at all, I'm using the grant mapping
> hypercall here.
> 

Oops, my bad! Ignore that one.

Wei.

> Regards,
> 
> Zoli

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