On 12.09.2017 14:28, Christian Borntraeger wrote:
>
>
> On 09/01/2017 05:11 PM, David Hildenbrand wrote:
>
>> @@ -466,11 +458,7 @@ static int pin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t *hpa)
>> /* Unpins a page previously pinned via pin_guest_page, marking it as dirty. */
>> static void unpin_guest_page(struct kvm *kvm, gpa_t gpa, hpa_t hpa)
>> {
>> - struct page *page;
>> -
>> - page = virt_to_page(hpa);
>> - set_page_dirty_lock(page);
>> - put_page(page);
>> + kvm_release_pfn_dirty(hpa >> PAGE_SHIFT);
>> /* mark the page always as dirty for migration */
>> mark_page_dirty(kvm, gpa_to_gfn(gpa));
>> }
>
> This is probably ok but can you maybe outline why we no longer need the
> _lock variant of set_page_dirty? (Or asked differently: why did we use
> the _locked varaint)
>
> Other than that everything looks sane, I am just not sure about this part.
>
Short answer: As x86 uses this heavily, I was expecting it to do the
right thing. I can't sport any additional page locking in x86 code.
Long answer:
mm/page-writeback.c: It only seems to be relevant for !anonymous mappings.
"set_page_dirty() is racy [...] This is because another CPU could
truncate the page off the mapping and then free the mapping."
"For pages with a mapping this should be done under the page lock for
the benefit of asynchronous memory errors who prefer a consistent dirty
state. This rule can be broken in some special cases [...]"
Sounds to me like the worst thing that could happen is losing a dirty
flag. Which should not matter if somebody tries to do nasty stuff with
the mapping either way.
But to verify: Paolo, can you recall why we don't need the _locked
variants in kvm common code?
--
Thanks,
David
