[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V3] xen/privcmd: improve performance of mapping of guest memory to dom0



On 22/11/12 08:42, Jan Beulich wrote:
On 21.11.12 at 18:19, Mats Petersson <mats.petersson@xxxxxxxxxx> wrote:
@@ -2526,19 +2557,94 @@ int xen_remap_domain_mfn_range(struct vm_area_struct
*vma,
                if (err)
                        goto out;
- err = HYPERVISOR_mmu_update(mmu_update, batch, NULL, domid);
-               if (err < 0)
-                       goto out;
+               /* We record the error for each page that gives an error, but
+                * continue mapping until the whole set is done */
+               do {
+                       err = HYPERVISOR_mmu_update(&mmu_update[index],
+                                                   batch_left, &done, domid);
+                       if (err < 0) {
+                               /* incrememnt done so we skip the error item */
increment

+                               done++;
+                               if (err_ptr)
+                                       last_err = err_ptr[index] = err;
+                               else
+                                       /* exit if error and no err_ptr */
+                                       goto out;
For readability/reduction of indentation, I'd suggest

Yes, that's neater (spent far too long programming in environments where goto was frowned upon, so I end up taking different routes that don't make best use of it...)


                                /* exit if error and no err_ptr */
                                if (!err_ptr)
                                        goto out;
                                /* increment done so we skip the error item */
                                done++;
                                last_err = err_ptr[index] = err;

However, I wonder how a caller of the function would find out
at which slot the error happened when not passing an error
indicator array (after all, the function also doesn't undo what
already succeeded in that case, i.e. the state is completely
unknown to such a caller).
I agree, which is why the use of err_ptr is the preferred method - I have talked to Ian Campbell, and he agrees that the "old" interface, which doesn't use the err_ptr, should be removed eventually. However, we first have to remove the callers of that method, and since it's a different interface.

However, current callers of the "non-errptr" interface already has this problem regardless of these changes - there is no indication of how far it got in the existing code either, it just exits with an error code if one happens. Nothing has changed here except the code to fill in the err_ptr moved from privcmd.c to mmu.c. If the caller detects a fail, it will (or should, but I'm not aware of any code that doesn't at present) call munmap the region, which undoes the entire mapping.

We can't, at this point undo things, as we can't know how many previous blocks of mapping has been done to this VMA. The owner of the memory region may well have mappe several "bunches" of MFN's before the error, which this function only handles one "bunch" at the time. So it's up to the caller to deal with the error appropriately.

In summary, I don't see a reason to change it. And the caller still needs to call munmap to destroy the memory region it is mapping the MFNs into, so it wouldn't be much help to "undo" things here.

--
Mats


Jan

+                       }
+                       batch_left -= done;
+                       index += done;
+               } while (batch_left);



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.