Yang, Xiaowei
2009-Mar-20 05:05 UTC
[Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
DIRECTMAP_VIRT_END can''t be passed to virt_to_mfn(), as it''s just beyond direct map boundary and triggers ASSERT very early at boot time. Signed-off-by: Xiaowei Yang <xiaowei.yang@intel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Mar-20 08:15 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>> >DIRECTMAP_VIRT_END can''t be passed to virt_to_mfn(), as it''s just beyond >direct map boundary and triggers ASSERT very early at boot time.While I agree to the analysis, I would think that this + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) ) should rather be + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 ) Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yang, Xiaowei
2009-Mar-20 08:34 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
Jan Beulich wrote:>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>> >> DIRECTMAP_VIRT_END can''t be passed to virt_to_mfn(), as it''s just beyond >> direct map boundary and triggers ASSERT very early at boot time. > > While I agree to the analysis, I would think that this > > + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) ) > > should rather be > > + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 ) >virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use ''<='' here. Thanks, xiaowei _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Mar-20 08:47 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 09:34 >>> >Jan Beulich wrote: >>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>> >>> DIRECTMAP_VIRT_END can''t be passed to virt_to_mfn(), as it''s just beyond >>> direct map boundary and triggers ASSERT very early at boot time. >> >> While I agree to the analysis, I would think that this >> >> + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) ) >> >> should rather be >> >> + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 ) >> >virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal toDepending on whether DIRECTMAP_VIRT_END is the last byte or the first following byte. Using "- 1" avoids such a dependency.>virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use ''<='' here.Because on the left side of the comparison we also calculate the first following mfn, not the last included one. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Mar-20 08:56 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
On 20/03/2009 08:47, "Jan Beulich" <jbeulich@novell.com> wrote:> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to > > Depending on whether DIRECTMAP_VIRT_END is the last byte or the first > following byte. Using "- 1" avoids such a dependency. > >> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use ''<='' here. > > Because on the left side of the comparison we also calculate the first > following mfn, not the last included one.Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it sounds like you think the page_alloc.c chunk is not needed at all. I''ll just check in the header fix for now. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Mar-20 09:27 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
>>> Keir Fraser <keir.fraser@eu.citrix.com> 20.03.09 09:56 >>> >On 20/03/2009 08:47, "Jan Beulich" <jbeulich@novell.com> wrote: > >> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to >> >> Depending on whether DIRECTMAP_VIRT_END is the last byte or the first >> following byte. Using "- 1" avoids such a dependency. >> >>> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use ''<='' here. >> >> Because on the left side of the comparison we also calculate the first >> following mfn, not the last included one. > >Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it >sounds like you think the page_alloc.c chunk is not needed at all. I''ll just >check in the header fix for now.No, that chunk is necessary afaics - as I said I agree to Xiaowei''s diagnosis that virt_to_mfn() should not be used on DIRECTMAP_VIRT_END alone. Subtracting 1 from it, and then adding back 1 to the result is indeed what I think is the right thing to do here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2009-Mar-20 09:32 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
On 20/03/2009 09:27, "Jan Beulich" <jbeulich@novell.com> wrote:>> Since DIRECTMAP_VIRT_END is always the following byte not the last byte, it >> sounds like you think the page_alloc.c chunk is not needed at all. I''ll just >> check in the header fix for now. > > No, that chunk is necessary afaics - as I said I agree to Xiaowei''s diagnosis > that virt_to_mfn() should not be used on DIRECTMAP_VIRT_END alone. > Subtracting 1 from it, and then adding back 1 to the result is indeed what > I think is the right thing to do here.Ah... virt_to_mfn() has a discontinuity at DIRECTMAP_VIRT_END. I get you. I''ll apply that chunk as a separate changeset then. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yang, Xiaowei
2009-Mar-24 10:48 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
Jan Beulich wrote:>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 09:34 >>> >> Jan Beulich wrote: >>>>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 20.03.09 06:05 >>> >>>> DIRECTMAP_VIRT_END can''t be passed to virt_to_mfn(), as it''s just beyond >>>> direct map boundary and triggers ASSERT very early at boot time. >>> While I agree to the analysis, I would think that this >>> >>> + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE) ) >>> >>> should rather be >>> >>> + mfn + needed <= virt_to_mfn(DIRECTMAP_VIRT_END - 1) + 1 ) >>> >> virt_to_mfn(DIRECTMAP_VIRT_END - 1) is equal to > > Depending on whether DIRECTMAP_VIRT_END is the last byte or the first > following byte. Using "- 1" avoids such a dependency. > >> virt_to_mfn(DIRECTMAP_VIRT_END - PAGE_SIZE). Why +1? We use ''<='' here. > > Because on the left side of the comparison we also calculate the first > following mfn, not the last included one. >Jan, Thanks for the clarification! Now I found another potential issue. Since mfn+needed could be equal to virt_to_mfn(DIRECTMAP_VIRT_END-1)+1, it can''t be passed to mfn_to_virt() directly - the valid range is [0,DIRECT_MAP_BYTES>>PAGE_SHIFT). Here is the patch to fix it. More assert and 32 bit counterpart are added. Signed-off-by: Xiaowei Yang <xiaowei.yang@intel.com> Thanks, xiaowei _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2009-Mar-24 10:55 UTC
Re: [Xen-devel] [PATCH] Fix domheap structure allocation when NUMA=on
>>> "Yang, Xiaowei" <xiaowei.yang@intel.com> 24.03.09 11:48 >>> >Now I found another potential issue. Since mfn+needed could be equal to >virt_to_mfn(DIRECTMAP_VIRT_END-1)+1, it can''t be passed to mfn_to_virt() >directly - the valid range is [0,DIRECT_MAP_BYTES>>PAGE_SHIFT).Oh, indeed. Thanks for noticing and fixing. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel