| Message ID | 573201D202000078000EA201@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.05.16 at 15:44, <JBeulich@suse.com> wrote: > Reclaiming pages is pointless when the cache can already satisfy all > outstanding PoD entries, and doing reclaims in that case can be very > harmful to performance when that memory gets used by the guest, but > only to store zeroes there. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> And I realize I forgot to Cc you, Wei. Jan > --- a/xen/arch/x86/mm/p2m-pod.c > +++ b/xen/arch/x86/mm/p2m-pod.c > @@ -1027,7 +1027,6 @@ static void pod_eager_record(struct p2m_ > { > struct pod_mrp_list *mrp = &p2m->pod.mrp; > > - ASSERT(mrp->list[mrp->idx] == INVALID_GFN); > ASSERT(gfn != INVALID_GFN); > > mrp->list[mrp->idx++] = > @@ -1075,7 +1074,9 @@ p2m_pod_demand_populate(struct p2m_domai > return 0; > } > > - pod_eager_reclaim(p2m); > + /* Only reclaim if we're in actual need of more cache. */ > + if ( p2m->pod.entry_count > p2m->pod.count ) > + pod_eager_reclaim(p2m); > > /* Only sweep if we're actually out of memory. Doing anything else > * causes unnecessary time and fragmentation of superpages in the p2m. > */
On 10/05/16 14:44, Jan Beulich wrote: > Reclaiming pages is pointless when the cache can already satisfy all > outstanding PoD entries, and doing reclaims in that case can be very > harmful to performance when that memory gets used by the guest, but > only to store zeroes there. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Possibly worth mentioning a dd from /dev/zero as the most likely action to trigger this.
On Tue, May 10, 2016 at 07:49:01AM -0600, Jan Beulich wrote: > >>> On 10.05.16 at 15:44, <JBeulich@suse.com> wrote: > > Reclaiming pages is pointless when the cache can already satisfy all > > outstanding PoD entries, and doing reclaims in that case can be very > > harmful to performance when that memory gets used by the guest, but > > only to store zeroes there. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > And I realize I forgot to Cc you, Wei. > Release-acked-by: Wei Liu <wei.liu2@citrix.com> Please apply this after we cut RC2. Wei.
>>> On 10.05.16 at 15:50, <andrew.cooper3@citrix.com> wrote: > On 10/05/16 14:44, Jan Beulich wrote: >> Reclaiming pages is pointless when the cache can already satisfy all >> outstanding PoD entries, and doing reclaims in that case can be very >> harmful to performance when that memory gets used by the guest, but >> only to store zeroes there. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Possibly worth mentioning a dd from /dev/zero as the most likely action > to trigger this. Well, I didn't want to because I think there are other means, like mmap()-ing anonymous memory. Jan
On 10/05/16 14:44, Jan Beulich wrote: > Reclaiming pages is pointless when the cache can already satisfy all > outstanding PoD entries, and doing reclaims in that case can be very > harmful to performance when that memory gets used by the guest, but > only to store zeroes there. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Sorry for the delay. Just one question... > > --- a/xen/arch/x86/mm/p2m-pod.c > +++ b/xen/arch/x86/mm/p2m-pod.c > @@ -1027,7 +1027,6 @@ static void pod_eager_record(struct p2m_ > { > struct pod_mrp_list *mrp = &p2m->pod.mrp; > > - ASSERT(mrp->list[mrp->idx] == INVALID_GFN); > ASSERT(gfn != INVALID_GFN); What's this for? -George
>>> On 12.05.16 at 16:40, <george.dunlap@citrix.com> wrote: > On 10/05/16 14:44, Jan Beulich wrote: >> Reclaiming pages is pointless when the cache can already satisfy all >> outstanding PoD entries, and doing reclaims in that case can be very >> harmful to performance when that memory gets used by the guest, but >> only to store zeroes there. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Sorry for the delay. Just one question... > >> >> --- a/xen/arch/x86/mm/p2m-pod.c >> +++ b/xen/arch/x86/mm/p2m-pod.c >> @@ -1027,7 +1027,6 @@ static void pod_eager_record(struct p2m_ >> { >> struct pod_mrp_list *mrp = &p2m->pod.mrp; >> >> - ASSERT(mrp->list[mrp->idx] == INVALID_GFN); >> ASSERT(gfn != INVALID_GFN); > > What's this for? The deletion of the ASSERT()? Since we no longer always insert a GFN through pod_eager_reclaim(), we also can no longer assume all (used) entries hold a valid GFN. Jan
On 12/05/16 16:08, Jan Beulich wrote: >>>> On 12.05.16 at 16:40, <george.dunlap@citrix.com> wrote: >> On 10/05/16 14:44, Jan Beulich wrote: >>> Reclaiming pages is pointless when the cache can already satisfy all >>> outstanding PoD entries, and doing reclaims in that case can be very >>> harmful to performance when that memory gets used by the guest, but >>> only to store zeroes there. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> Sorry for the delay. Just one question... >> >>> >>> --- a/xen/arch/x86/mm/p2m-pod.c >>> +++ b/xen/arch/x86/mm/p2m-pod.c >>> @@ -1027,7 +1027,6 @@ static void pod_eager_record(struct p2m_ >>> { >>> struct pod_mrp_list *mrp = &p2m->pod.mrp; >>> >>> - ASSERT(mrp->list[mrp->idx] == INVALID_GFN); >>> ASSERT(gfn != INVALID_GFN); >> >> What's this for? > > The deletion of the ASSERT()? Since we no longer always insert a > GFN through pod_eager_reclaim(), we also can no longer assume > all (used) entries hold a valid GFN. Oh, right. Should have taken a closer look, sorry for the noise: Reviewed-by: George Dunlap <george.dunlap@citrix.com>
--- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -1027,7 +1027,6 @@ static void pod_eager_record(struct p2m_ { struct pod_mrp_list *mrp = &p2m->pod.mrp; - ASSERT(mrp->list[mrp->idx] == INVALID_GFN); ASSERT(gfn != INVALID_GFN); mrp->list[mrp->idx++] = @@ -1075,7 +1074,9 @@ p2m_pod_demand_populate(struct p2m_domai return 0; } - pod_eager_reclaim(p2m); + /* Only reclaim if we're in actual need of more cache. */ + if ( p2m->pod.entry_count > p2m->pod.count ) + pod_eager_reclaim(p2m); /* Only sweep if we're actually out of memory. Doing anything else * causes unnecessary time and fragmentation of superpages in the p2m. */