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! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html