On Monday 19 Jul 2021 at 13:14:48 (+0100), Marc Zyngier wrote: > On Mon, 19 Jul 2021 11:47:24 +0100, > Quentin Perret <qperret@xxxxxxxxxx> wrote: > > > > The stage-2 map walkers currently return -EAGAIN when re-creating > > identical mappings or only changing access permissions. This allows to > > optimize mapping pages for concurrent (v)CPUs faulting on the same > > page. > > > > While this works as expected when touching one page-table leaf at a > > time, this can lead to difficult situations when mapping larger ranges. > > Indeed, a large map operation can fail in the middle if an existing > > mapping is found in the range, even if it has compatible attributes, > > hence leaving only half of the range mapped. > > I'm curious of when this can happen. We normally map a single leaf at > a time, and we don't have a way to map multiple leaves at once: we > either use the VMA base size or try to upgrade it to a THP, but the > result is always a single leaf entry. What changed? Nothing _yet_ :-) The 'share' hypercall introduced near the end of the series allows to share multiple physically contiguous pages in one go -- this is mostly to allow sharing data-structures that are larger than a page. So if one of the pages happens to be already mapped by the time the hypercall is issued, mapping the range with the right SW bits becomes difficult as kvm_pgtable_stage2_map() will fail halfway through, which is tricky to handle. This patch shouldn't change anything for existing users that only map things that are nicely aligned at block/page granularity, but should make the life of new users easier, so that seemed like a win. > > To avoid having to deal with such failures in the caller, don't > > interrupt the map operation when hitting existing PTEs, but make sure to > > still return -EAGAIN so that user_mem_abort() can mark the page dirty > > when needed. > > I don't follow you here: if you return -EAGAIN for a writable mapping, > we don't account for the page to be dirty on the assumption that > nothing has been mapped. But if there is a way to map more than a > single entry and to get -EAGAIN at the same time, then we're bound to > lose data on page eviction. > > Can you shed some light on this? Sure. For guests, hitting the -EAGAIN case means we've lost the race with another vCPU that faulted the same page. In this case the other vCPU either mapped the page RO, which means that our vCPU will then get a permission fault next time we run it which will lead to the page being marked dirty, or the other vCPU mapped the page RW in which case it already marked the page dirty for us and we can safely re-enter the guest without doing anything else. So what I meant by "still return -EAGAIN so that user_mem_abort() can mark the page dirty when needed" is "make sure to mark the page dirty only when necessary: if winning the race and marking the page RW, or in the permission fault path". That is, by keeping the -EAGAIN I want to make sure we don't mark the page dirty twice. (This might fine, but this would be new behaviour, and it was not clear that would scale well to many vCPUs faulting the same page). I see how this wording can be highly confusing though, I'll and re-word for the next version. Cheers, Quentin _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
