| Message ID | 17ea032201adfecdb5dedc3a0b9667eccdf7f118.1579628566.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 21.01.2020 18:49, Tamas K Lengyel wrote: > The owner domain of shared pages is dom_cow, use that for get_page > otherwise the function fails to return the correct page. I think this description needs improvement: The function does the special shared page dance in one place (on the "fast path") already. This wants mentioning, either because it was a mistake to have it just there, or because a new need has appeared to also have it on the "slow path". > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( > if ( p2m_is_ram(*t) && mfn_valid(mfn) ) > { > page = mfn_to_page(mfn); > - if ( !get_page(page, p2m->domain) ) > + if ( !get_page(page, p2m->domain) && > + /* Page could be shared */ > + (!dom_cow || !p2m_is_shared(*t) || > + !get_page(page, dom_cow)) ) While there may be a reason why on the fast path two get_page() invocations are be necessary, couldn't you get away with just one if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain : dom_cow) ) at least here? It's also not really clear to me why here and there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't p2m_is_shared() return consistently "false" when !dom_cow ? Jan
On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 21.01.2020 18:49, Tamas K Lengyel wrote: > > The owner domain of shared pages is dom_cow, use that for get_page > > otherwise the function fails to return the correct page. > > I think this description needs improvement: The function does the > special shared page dance in one place (on the "fast path") > already. This wants mentioning, either because it was a mistake > to have it just there, or because a new need has appeared to also > have it on the "slow path". It was a pre-existing error not to get the page from dom_cow for a shared entry in the slow path. I only ran into it now because the erroneous type_count check move in the previous version of the series was resulting in all pages being fully deduplicated instead of mostly being shared. Now that the pages are properly shared running LibVMI on the fork resulted in failures do to this bug. > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( > > if ( p2m_is_ram(*t) && mfn_valid(mfn) ) > > { > > page = mfn_to_page(mfn); > > - if ( !get_page(page, p2m->domain) ) > > + if ( !get_page(page, p2m->domain) && > > + /* Page could be shared */ > > + (!dom_cow || !p2m_is_shared(*t) || > > + !get_page(page, dom_cow)) ) > > While there may be a reason why on the fast path two get_page() > invocations are be necessary, couldn't you get away with just > one > > if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain > : dom_cow) ) > > at least here? It's also not really clear to me why here and > there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't > p2m_is_shared() return consistently "false" when !dom_cow ? I simply copied the existing code from the slow_path as-is. IMHO it would suffice to do a single get_page(page, !p2m_is_shared(*t) ? p2m->domain : dom_cow); since we can't have any entries that are shared when dom_cow is NULL so this is safe, no need for the extra !dom_cow check. If you prefer I can make the change for both locations. Tamas
On 22.01.2020 17:51, Tamas K Lengyel wrote: > On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 21.01.2020 18:49, Tamas K Lengyel wrote: >>> The owner domain of shared pages is dom_cow, use that for get_page >>> otherwise the function fails to return the correct page. >> >> I think this description needs improvement: The function does the >> special shared page dance in one place (on the "fast path") >> already. This wants mentioning, either because it was a mistake >> to have it just there, or because a new need has appeared to also >> have it on the "slow path". > > It was a pre-existing error not to get the page from dom_cow for a > shared entry in the slow path. I only ran into it now because the > erroneous type_count check move in the previous version of the series > was resulting in all pages being fully deduplicated instead of mostly > being shared. Now that the pages are properly shared running LibVMI on > the fork resulted in failures do to this bug. > >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( >>> if ( p2m_is_ram(*t) && mfn_valid(mfn) ) >>> { >>> page = mfn_to_page(mfn); >>> - if ( !get_page(page, p2m->domain) ) >>> + if ( !get_page(page, p2m->domain) && >>> + /* Page could be shared */ >>> + (!dom_cow || !p2m_is_shared(*t) || >>> + !get_page(page, dom_cow)) ) >> >> While there may be a reason why on the fast path two get_page() >> invocations are be necessary, couldn't you get away with just >> one >> >> if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain >> : dom_cow) ) >> >> at least here? It's also not really clear to me why here and >> there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't >> p2m_is_shared() return consistently "false" when !dom_cow ? > > I simply copied the existing code from the slow_path as-is. IMHO it > would suffice to do a single get_page(page, !p2m_is_shared(*t) ? > p2m->domain : dom_cow); since we can't have any entries that are > shared when dom_cow is NULL so this is safe, no need for the extra > !dom_cow check. If you prefer I can make the change for both > locations. If the change is correct to make also in the other place, I'd definitely prefer you doing so. Jan
On 1/22/20 4:55 PM, Jan Beulich wrote: > On 22.01.2020 17:51, Tamas K Lengyel wrote: >> On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 21.01.2020 18:49, Tamas K Lengyel wrote: >>>> The owner domain of shared pages is dom_cow, use that for get_page >>>> otherwise the function fails to return the correct page. >>> >>> I think this description needs improvement: The function does the >>> special shared page dance in one place (on the "fast path") >>> already. This wants mentioning, either because it was a mistake >>> to have it just there, or because a new need has appeared to also >>> have it on the "slow path". >> >> It was a pre-existing error not to get the page from dom_cow for a >> shared entry in the slow path. I only ran into it now because the >> erroneous type_count check move in the previous version of the series >> was resulting in all pages being fully deduplicated instead of mostly >> being shared. Now that the pages are properly shared running LibVMI on >> the fork resulted in failures do to this bug. >> >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>>> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( >>>> if ( p2m_is_ram(*t) && mfn_valid(mfn) ) >>>> { >>>> page = mfn_to_page(mfn); >>>> - if ( !get_page(page, p2m->domain) ) >>>> + if ( !get_page(page, p2m->domain) && >>>> + /* Page could be shared */ >>>> + (!dom_cow || !p2m_is_shared(*t) || >>>> + !get_page(page, dom_cow)) ) >>> >>> While there may be a reason why on the fast path two get_page() >>> invocations are be necessary, couldn't you get away with just >>> one >>> >>> if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain >>> : dom_cow) ) >>> >>> at least here? It's also not really clear to me why here and >>> there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't >>> p2m_is_shared() return consistently "false" when !dom_cow ? >> >> I simply copied the existing code from the slow_path as-is. IMHO it >> would suffice to do a single get_page(page, !p2m_is_shared(*t) ? >> p2m->domain : dom_cow); since we can't have any entries that are >> shared when dom_cow is NULL so this is safe, no need for the extra >> !dom_cow check. If you prefer I can make the change for both >> locations. > > If the change is correct to make also in the other place, I'd > definitely prefer you doing so. I agree with everything Jan said. :-) Also, since this is a general bugfix, you might consider moving it to the top of your series, so it can be checked in as soon as it's ready. -George
On Thu, Jan 23, 2020 at 8:32 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 1/22/20 4:55 PM, Jan Beulich wrote: > > On 22.01.2020 17:51, Tamas K Lengyel wrote: > >> On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@suse.com> wrote: > >>> > >>> On 21.01.2020 18:49, Tamas K Lengyel wrote: > >>>> The owner domain of shared pages is dom_cow, use that for get_page > >>>> otherwise the function fails to return the correct page. > >>> > >>> I think this description needs improvement: The function does the > >>> special shared page dance in one place (on the "fast path") > >>> already. This wants mentioning, either because it was a mistake > >>> to have it just there, or because a new need has appeared to also > >>> have it on the "slow path". > >> > >> It was a pre-existing error not to get the page from dom_cow for a > >> shared entry in the slow path. I only ran into it now because the > >> erroneous type_count check move in the previous version of the series > >> was resulting in all pages being fully deduplicated instead of mostly > >> being shared. Now that the pages are properly shared running LibVMI on > >> the fork resulted in failures do to this bug. > >> > >>>> --- a/xen/arch/x86/mm/p2m.c > >>>> +++ b/xen/arch/x86/mm/p2m.c > >>>> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( > >>>> if ( p2m_is_ram(*t) && mfn_valid(mfn) ) > >>>> { > >>>> page = mfn_to_page(mfn); > >>>> - if ( !get_page(page, p2m->domain) ) > >>>> + if ( !get_page(page, p2m->domain) && > >>>> + /* Page could be shared */ > >>>> + (!dom_cow || !p2m_is_shared(*t) || > >>>> + !get_page(page, dom_cow)) ) > >>> > >>> While there may be a reason why on the fast path two get_page() > >>> invocations are be necessary, couldn't you get away with just > >>> one > >>> > >>> if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain > >>> : dom_cow) ) > >>> > >>> at least here? It's also not really clear to me why here and > >>> there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't > >>> p2m_is_shared() return consistently "false" when !dom_cow ? > >> > >> I simply copied the existing code from the slow_path as-is. IMHO it > >> would suffice to do a single get_page(page, !p2m_is_shared(*t) ? > >> p2m->domain : dom_cow); since we can't have any entries that are > >> shared when dom_cow is NULL so this is safe, no need for the extra > >> !dom_cow check. If you prefer I can make the change for both > >> locations. > > > > If the change is correct to make also in the other place, I'd > > definitely prefer you doing so. > > I agree with everything Jan said. :-) > > Also, since this is a general bugfix, you might consider moving it to > the top of your series, so it can be checked in as soon as it's ready. Sure - although it can be checked in before patch 1 & 2, it's not dependent on them. In fact, all the patches in this series up to and including Patch 14 could be checked in out-of-order. Tamas
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 3119269073..fdeb742707 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn( if ( p2m_is_ram(*t) && mfn_valid(mfn) ) { page = mfn_to_page(mfn); - if ( !get_page(page, p2m->domain) ) + if ( !get_page(page, p2m->domain) && + /* Page could be shared */ + (!dom_cow || !p2m_is_shared(*t) || + !get_page(page, dom_cow)) ) page = NULL; } put_gfn(p2m->domain, gfn_x(gfn));
The owner domain of shared pages is dom_cow, use that for get_page otherwise the function fails to return the correct page. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/p2m.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)