Messages in this thread | | | Date | Thu, 26 Jul 2012 17:25:07 +0800 | From | Xiao Guangrong <> | Subject | Re: [PATCH v2 3/3] KVM: remove dummy pages |
| |
On 07/26/2012 04:56 PM, Avi Kivity wrote: > On 07/26/2012 06:58 AM, Xiao Guangrong wrote: >> Currently, kvm allocates some pages and use them as error indicators, >> it wastes memory and is not good for scalability >> >> Base on Avi's suggestion, we use the error codes instead of these pages >> to indicate the error conditions >> >> >> +static pfn_t get_bad_pfn(void) >> +{ >> + return -ENOENT; >> +} >> + >> +pfn_t get_fault_pfn(void) >> +{ >> + return -EFAULT; >> +} >> +EXPORT_SYMBOL_GPL(get_fault_pfn); >> + >> +static pfn_t get_hwpoison_pfn(void) >> +{ >> + return -EHWPOISON; >> +} >> + > > Would be better as #defines
Okay.
> >> int is_hwpoison_pfn(pfn_t pfn) >> { >> - return pfn == hwpoison_pfn; >> + return pfn == -EHWPOISON; >> } >> EXPORT_SYMBOL_GPL(is_hwpoison_pfn); >> >> int is_noslot_pfn(pfn_t pfn) >> { >> - return pfn == bad_pfn; >> + return pfn == -ENOENT; >> } >> EXPORT_SYMBOL_GPL(is_noslot_pfn); >> >> int is_invalid_pfn(pfn_t pfn) >> { >> - return pfn == hwpoison_pfn || pfn == fault_pfn; >> + return !is_noslot_pfn(pfn) && is_error_pfn(pfn); >> } >> EXPORT_SYMBOL_GPL(is_invalid_pfn); >> > > So is_*_pfn() could go away and be replaced by ==. >
Okay.
>> >> EXPORT_SYMBOL_GPL(gfn_to_page); >> >> void kvm_release_page_clean(struct page *page) >> { >> - kvm_release_pfn_clean(page_to_pfn(page)); >> + if (!is_error_page(page)) >> + kvm_release_pfn_clean(page_to_pfn(page)); >> } >> EXPORT_SYMBOL_GPL(kvm_release_page_clean); > > Note, we can remove calls to kvm_release_page_clean() from error paths > now, so in the future we can drop the test. >
Right, since the release path (kvm_release_page_clean) is used in many place and on many architectures, i did the change as small as possible for good review.
> Since my comments are better done as a separate patch,
Yes, i will make a patch to apply all your comments. Thank you, Avi!
| |