From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mats Petersson Subject: Re: [PATCH] Improve performance of IOCTL_PRIVCMD_MMAPBATCH_V2 Date: Wed, 21 Nov 2012 10:51:26 +0000 Message-ID: <50ACB22E.4070101@citrix.com> References: <1353077152-29630-1-git-send-email-mats.petersson@citrix.com> <50ACBB1A02000078000AA4E2@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50ACBB1A02000078000AA4E2@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "konrad.wilk@oracle.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 21/11/12 10:29, Jan Beulich wrote: >>>> On 16.11.12 at 15:45, Mats Petersson wrote: >> @@ -2526,12 +2540,25 @@ 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) { >> + if (err_ptr) >> + err_ptr[index] = err; > Shouldn't you increment "done" here, in order to not retry the failed > slot immediately? Yes, good spot - for some reason, I have double checked the behaviour of "done", and it returns the index of the item which gave the error, not actually "how many were processed". I have rewritten this part of code for V3 of this patch, but I think it still requires an increment of "done" to make it work correctly. > >> + else /* exit if error and no err_ptr */ >> + goto out; >> + } >> + batch_left -= done; >> + index += done; >> + } while (batch_left); >> >> nr -= batch; >> addr += range; >> + if (err_ptr) >> + err_ptr += batch; >> } >> >> err = 0; >> @@ -303,6 +349,8 @@ static int mmap_return_errors_v1(void *data, void *state) >> return __put_user(*mfnp, st->user_mfn++); >> } >> >> + >> + > ??? This is cleaned up in the V3 patch. >> static struct vm_operations_struct privcmd_vm_ops; >> >> static long privcmd_ioctl_mmap_batch(void __user *udata, int version) >> @@ -319,6 +367,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, >> int version) >> if (!xen_initial_domain()) >> return -EPERM; >> >> + >> + > ??? As above. -- Mats > Jan > >> switch (version) { >> case 1: >> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch))) > > >