[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: 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...)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 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./* 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). 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |