On 10/01/14 15:24, Zoltan Kiss wrote:

On 10/01/14 11:45, Wei Liu wrote:

quoted

On Fri, Jan 10, 2014 at 11:35:08AM +0000, Zoltan Kiss wrote:
[...]

quoted

quoted

quoted

@@ -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.

I agree, as I mentioned in other email in this thread, I think that
should be the topic of an another patchseries. In the meantime, I will
use gnttab_batch_map instead of the direct hypercall, it handles the
GNTST_eagain scenario, and I will use set_phys_to_machine the same way
as m2p_override does:

If the grant table code doesn't provide the API calls you need you can
either:

a) add the new API as a prerequisite patch.
b) use the existing API calls and live with the performance problem,
until you can refactor the API later on.

Adding a netback-specific hack isn't a valid option.

David