Prev: Bluetooth: Fix abuse of the preincrement operator
Next: [PATCH 2/2] Removing dead CONFIG_IOMMU_DEBUG
From: Xiao Guangrong on 30 Jun 2010 21:20


Marcelo Tosatti wrote:

>> +
>> + addr = gfn_to_hva_many(vcpu->kvm, gfn, &entry);
>> + if (kvm_is_error_hva(addr))
>> + return -1;
>> +
>> + entry = min(entry, (int)(end - start));
>> + ret = __get_user_pages_fast(addr, entry, 1, pages);
>> + if (ret <= 0)
>> + return -1;
>
> Why can't you use gfn_to_pfn_atomic() here, one page at a time? Is
> the overhead significant that this is worthwhile?
>
> You're bypassing the centralized interface.

I think it's worthwhile to do since we can reduce gup overhead, no reason
to traverse process's page table again and again for the consecutive pages.

>
>> +
>> + pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
>> + mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
>> + is_dirty_gpte(gpte), NULL, sp->role.level, gfn,
>> + pfn, true, false);
>
> reset_host_protection should be true, see commit 1403283acca (also for
> direct case to be consistent).
>

Ah, i forgot it, thanks, Marcelo!

I'll post a small fix patch base on this patchset to change 'false' to 'true' if no
other places no to be improved. :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Marcelo Tosatti on 1 Jul 2010 08:10
On Thu, Jul 01, 2010 at 09:11:36AM +0800, Xiao Guangrong wrote:
>
>
> Marcelo Tosatti wrote:
>
> >> +
> >> + addr = gfn_to_hva_many(vcpu->kvm, gfn, &entry);
> >> + if (kvm_is_error_hva(addr))
> >> + return -1;
> >> +
> >> + entry = min(entry, (int)(end - start));
> >> + ret = __get_user_pages_fast(addr, entry, 1, pages);
> >> + if (ret <= 0)
> >> + return -1;
> >
> > Why can't you use gfn_to_pfn_atomic() here, one page at a time? Is
> > the overhead significant that this is worthwhile?
> >
> > You're bypassing the centralized interface.
>
> I think it's worthwhile to do since we can reduce gup overhead, no reason
> to traverse process's page table again and again for the consecutive pages.

I'd prefer to use gfn_to_pfn_atomic. The big win is with shadow mode and
there its already done one at a time.

> >
> >> +
> >> + pte_access = sp->role.access & FNAME(gpte_access)(vcpu, gpte);
> >> + mmu_set_spte(vcpu, spte, sp->role.access, pte_access, 0, 0,
> >> + is_dirty_gpte(gpte), NULL, sp->role.level, gfn,
> >> + pfn, true, false);
> >
> > reset_host_protection should be true, see commit 1403283acca (also for
> > direct case to be consistent).
> >
>
> Ah, i forgot it, thanks, Marcelo!
>
> I'll post a small fix patch base on this patchset to change 'false' to 'true' if no
> other places no to be improved. :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Avi Kivity on 1 Jul 2010 08:20
On 07/01/2010 04:11 AM, Xiao Guangrong wrote:
>
> Marcelo Tosatti wrote:
>
>
>>> +
>>> + addr = gfn_to_hva_many(vcpu->kvm, gfn,&entry);
>>> + if (kvm_is_error_hva(addr))
>>> + return -1;
>>> +
>>> + entry = min(entry, (int)(end - start));
>>> + ret = __get_user_pages_fast(addr, entry, 1, pages);
>>> + if (ret<= 0)
>>> + return -1;
>>>
>> Why can't you use gfn_to_pfn_atomic() here, one page at a time? Is
>> the overhead significant that this is worthwhile?
>>
>> You're bypassing the centralized interface.
>>
> I think it's worthwhile to do since we can reduce gup overhead, no reason
> to traverse process's page table again and again for the consecutive pages.
>

Then we should make the centralized interface work in terms of multiple
pages, and write the single-page interfaces in terms of the multipage
interfaces.

--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Xiao Guangrong on 1 Jul 2010 08:20


Avi Kivity wrote:
> On 07/01/2010 04:11 AM, Xiao Guangrong wrote:
>>
>> Marcelo Tosatti wrote:
>>
>>
>>>> +
>>>> + addr = gfn_to_hva_many(vcpu->kvm, gfn,&entry);
>>>> + if (kvm_is_error_hva(addr))
>>>> + return -1;
>>>> +
>>>> + entry = min(entry, (int)(end - start));
>>>> + ret = __get_user_pages_fast(addr, entry, 1, pages);
>>>> + if (ret<= 0)
>>>> + return -1;
>>>>
>>> Why can't you use gfn_to_pfn_atomic() here, one page at a time? Is
>>> the overhead significant that this is worthwhile?
>>>
>>> You're bypassing the centralized interface.
>>>
>> I think it's worthwhile to do since we can reduce gup overhead, no reason
>> to traverse process's page table again and again for the consecutive
>> pages.
>>
>
> Then we should make the centralized interface work in terms of multiple
> pages, and write the single-page interfaces in terms of the multipage
> interfaces.
>

Umm, i'll import a new function named gfn_to_pfn_many_atomic(... int *enough),
using 'enough' to indicate whether have got the all consecutive pages in the slot,
Marcelo, how about it? :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Marcelo Tosatti on 1 Jul 2010 08:30
On Thu, Jul 01, 2010 at 08:13:40PM +0800, Xiao Guangrong wrote:
>
>
> Avi Kivity wrote:
> > On 07/01/2010 04:11 AM, Xiao Guangrong wrote:
> >>
> >> Marcelo Tosatti wrote:
> >>
> >>
> >>>> +
> >>>> + addr = gfn_to_hva_many(vcpu->kvm, gfn,&entry);
> >>>> + if (kvm_is_error_hva(addr))
> >>>> + return -1;
> >>>> +
> >>>> + entry = min(entry, (int)(end - start));
> >>>> + ret = __get_user_pages_fast(addr, entry, 1, pages);
> >>>> + if (ret<= 0)
> >>>> + return -1;
> >>>>
> >>> Why can't you use gfn_to_pfn_atomic() here, one page at a time? Is
> >>> the overhead significant that this is worthwhile?
> >>>
> >>> You're bypassing the centralized interface.
> >>>
> >> I think it's worthwhile to do since we can reduce gup overhead, no reason
> >> to traverse process's page table again and again for the consecutive
> >> pages.
> >>
> >
> > Then we should make the centralized interface work in terms of multiple
> > pages, and write the single-page interfaces in terms of the multipage
> > interfaces.
> >
>
> Umm, i'll import a new function named gfn_to_pfn_many_atomic(... int *enough),
> using 'enough' to indicate whether have got the all consecutive pages in the slot,
> Marcelo, how about it? :-)

Fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: Bluetooth: Fix abuse of the preincrement operator
Next: [PATCH 2/2] Removing dead CONFIG_IOMMU_DEBUG