| Message ID | 740404f0-3da9-4ae5-b4a5-b24cb2907e7d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] x86/PoD: tie together P2M update and increment of entry count | expand |
On Tue, Mar 12, 2024 at 3:22 PM Jan Beulich <jbeulich@suse.com> wrote: > > When not holding the PoD lock across the entire region covering P2M > update and stats update, the entry count - if to be incorrect at all - > should indicate too large a value in preference to a too small one, to > avoid functions bailing early when they find the count is zero. However, > instead of moving the increment ahead (and adjust back upon failure), > extend the PoD-locked region. > > Fixes: 99af3cd40b6e ("x86/mm: Rework locking in the PoD layer") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > The locked region could be shrunk again, by having multiple unlock > calls. But I think both ioreq_request_mapcache_invalidate() and > domain_crash() are fair enough to call with the lock still held? > --- > v3: Extend locked region instead. Add Fixes: tag. > v2: Add comments. > > --- a/xen/arch/x86/mm/p2m-pod.c > +++ b/xen/arch/x86/mm/p2m-pod.c > @@ -1348,16 +1348,22 @@ mark_populate_on_demand(struct domain *d > } > } > > + /* > + * P2M update and stats increment need to collectively be under PoD lock, > + * to prevent code elsewhere observing PoD entry count being zero despite > + * there actually still being PoD entries (created by the p2m_set_entry() > + * invocation below). > + */ > + pod_lock(p2m); > + > /* Now, actually do the two-way mapping */ > rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, > p2m_populate_on_demand, p2m->default_access); > if ( rc == 0 ) > { > - pod_lock(p2m); > p2m->pod.entry_count += 1UL << order; > p2m->pod.entry_count -= pod_count; > BUG_ON(p2m->pod.entry_count < 0); > - pod_unlock(p2m); > > ioreq_request_mapcache_invalidate(d); > } > @@ -1373,6 +1379,8 @@ mark_populate_on_demand(struct domain *d > domain_crash(d); > } > > + pod_unlock(p2m); We're confident that neither domain_crash() nor ioreq_request_mapcache_invalidate() will grab any of the p2m locks? If so, Reviewed-by: George Dunlap <george.dunlap@cloud.com>
On 13.03.2024 11:58, George Dunlap wrote: > On Tue, Mar 12, 2024 at 3:22 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> When not holding the PoD lock across the entire region covering P2M >> update and stats update, the entry count - if to be incorrect at all - >> should indicate too large a value in preference to a too small one, to >> avoid functions bailing early when they find the count is zero. However, >> instead of moving the increment ahead (and adjust back upon failure), >> extend the PoD-locked region. >> >> Fixes: 99af3cd40b6e ("x86/mm: Rework locking in the PoD layer") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> The locked region could be shrunk again, by having multiple unlock >> calls. But I think both ioreq_request_mapcache_invalidate() and >> domain_crash() are fair enough to call with the lock still held? >> --- >> v3: Extend locked region instead. Add Fixes: tag. >> v2: Add comments. >> >> --- a/xen/arch/x86/mm/p2m-pod.c >> +++ b/xen/arch/x86/mm/p2m-pod.c >> @@ -1348,16 +1348,22 @@ mark_populate_on_demand(struct domain *d >> } >> } >> >> + /* >> + * P2M update and stats increment need to collectively be under PoD lock, >> + * to prevent code elsewhere observing PoD entry count being zero despite >> + * there actually still being PoD entries (created by the p2m_set_entry() >> + * invocation below). >> + */ >> + pod_lock(p2m); >> + >> /* Now, actually do the two-way mapping */ >> rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, >> p2m_populate_on_demand, p2m->default_access); >> if ( rc == 0 ) >> { >> - pod_lock(p2m); >> p2m->pod.entry_count += 1UL << order; >> p2m->pod.entry_count -= pod_count; >> BUG_ON(p2m->pod.entry_count < 0); >> - pod_unlock(p2m); >> >> ioreq_request_mapcache_invalidate(d); >> } >> @@ -1373,6 +1379,8 @@ mark_populate_on_demand(struct domain *d >> domain_crash(d); >> } >> >> + pod_unlock(p2m); > > We're confident that neither domain_crash() nor > ioreq_request_mapcache_invalidate() will grab any of the p2m locks? There's no doubt about ioreq_request_mapcache_invalidate(). domain_crash(), otoh, invokes show_execution_state(), which in principle would be nice to dump the guest stack among other things. My patch doing so was reverted, so right now there's no issue there. Plus any attempt to do so would need to be careful anyway regarding locks. But as you see it is not a clear cut no, so ... > If so, > > Reviewed-by: George Dunlap <george.dunlap@cloud.com> ... rather than taking this (thanks), maybe I indeed better follow the alternative outlined in the post-commit-message remark? Jan
On Wed, Mar 13, 2024 at 12:19 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 13.03.2024 11:58, George Dunlap wrote: > > On Tue, Mar 12, 2024 at 3:22 PM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> When not holding the PoD lock across the entire region covering P2M > >> update and stats update, the entry count - if to be incorrect at all - > >> should indicate too large a value in preference to a too small one, to > >> avoid functions bailing early when they find the count is zero. However, > >> instead of moving the increment ahead (and adjust back upon failure), > >> extend the PoD-locked region. > >> > >> Fixes: 99af3cd40b6e ("x86/mm: Rework locking in the PoD layer") > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> The locked region could be shrunk again, by having multiple unlock > >> calls. But I think both ioreq_request_mapcache_invalidate() and > >> domain_crash() are fair enough to call with the lock still held? > >> --- > >> v3: Extend locked region instead. Add Fixes: tag. > >> v2: Add comments. > >> > >> --- a/xen/arch/x86/mm/p2m-pod.c > >> +++ b/xen/arch/x86/mm/p2m-pod.c > >> @@ -1348,16 +1348,22 @@ mark_populate_on_demand(struct domain *d > >> } > >> } > >> > >> + /* > >> + * P2M update and stats increment need to collectively be under PoD lock, > >> + * to prevent code elsewhere observing PoD entry count being zero despite > >> + * there actually still being PoD entries (created by the p2m_set_entry() > >> + * invocation below). > >> + */ > >> + pod_lock(p2m); > >> + > >> /* Now, actually do the two-way mapping */ > >> rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, > >> p2m_populate_on_demand, p2m->default_access); > >> if ( rc == 0 ) > >> { > >> - pod_lock(p2m); > >> p2m->pod.entry_count += 1UL << order; > >> p2m->pod.entry_count -= pod_count; > >> BUG_ON(p2m->pod.entry_count < 0); > >> - pod_unlock(p2m); > >> > >> ioreq_request_mapcache_invalidate(d); > >> } > >> @@ -1373,6 +1379,8 @@ mark_populate_on_demand(struct domain *d > >> domain_crash(d); > >> } > >> > >> + pod_unlock(p2m); > > > > We're confident that neither domain_crash() nor > > ioreq_request_mapcache_invalidate() will grab any of the p2m locks? > > There's no doubt about ioreq_request_mapcache_invalidate(). domain_crash(), > otoh, invokes show_execution_state(), which in principle would be nice to > dump the guest stack among other things. My patch doing so was reverted, so > right now there's no issue there. Plus any attempt to do so would need to > be careful anyway regarding locks. But as you see it is not a clear cut no, > so ... > > > If so, > > > > Reviewed-by: George Dunlap <george.dunlap@cloud.com> > > ... rather than taking this (thanks), maybe I indeed better follow the > alternative outlined in the post-commit-message remark? I keep missing your post-commit-message remarks due to the way I'm applying your series. Yes, that had occurred to me as well -- I don't think this is a hot path, and I do think it would be good to avoid laying a trap for future people wanting to change domain_crash(); in particular as that would change a domain crash into either a host crash or a potential deadlock. I think I would go with multiple if statements, rather than multiple unlock calls though. -George
On Wed, Mar 13, 2024 at 12:25 PM George Dunlap <george.dunlap@cloud.com> wrote: > I keep missing your post-commit-message remarks due to the way I'm > applying your series. Er, just to be clear, this is a problem with my workflow, not with your patches... -George
--- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -1348,16 +1348,22 @@ mark_populate_on_demand(struct domain *d } } + /* + * P2M update and stats increment need to collectively be under PoD lock, + * to prevent code elsewhere observing PoD entry count being zero despite + * there actually still being PoD entries (created by the p2m_set_entry() + * invocation below). + */ + pod_lock(p2m); + /* Now, actually do the two-way mapping */ rc = p2m_set_entry(p2m, gfn, INVALID_MFN, order, p2m_populate_on_demand, p2m->default_access); if ( rc == 0 ) { - pod_lock(p2m); p2m->pod.entry_count += 1UL << order; p2m->pod.entry_count -= pod_count; BUG_ON(p2m->pod.entry_count < 0); - pod_unlock(p2m); ioreq_request_mapcache_invalidate(d); } @@ -1373,6 +1379,8 @@ mark_populate_on_demand(struct domain *d domain_crash(d); } + pod_unlock(p2m); + out: gfn_unlock(p2m, gfn, order);
When not holding the PoD lock across the entire region covering P2M update and stats update, the entry count - if to be incorrect at all - should indicate too large a value in preference to a too small one, to avoid functions bailing early when they find the count is zero. However, instead of moving the increment ahead (and adjust back upon failure), extend the PoD-locked region. Fixes: 99af3cd40b6e ("x86/mm: Rework locking in the PoD layer") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- The locked region could be shrunk again, by having multiple unlock calls. But I think both ioreq_request_mapcache_invalidate() and domain_crash() are fair enough to call with the lock still held? --- v3: Extend locked region instead. Add Fixes: tag. v2: Add comments.