Mailing List Archive

[PATCH 5 of 6] [RFC] x86/mm: use wait queues for mem_paging
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 5 of 6] [RFC] x86/mm: use wait queues for mem_paging [ In reply to ]
On Thu, Feb 23, Tim Deegan wrote:

> This is based on an earlier RFC patch by Olaf Hering, but heavily
> simplified (removing a per-gfn queue of waiting vcpus in favour of
> a scan of all vcpus on page-in).

Tim, thanks for that work. From just reading the change it looks ok.

A few comments below:

> +++ b/xen/arch/x86/mm/p2m.c Thu Feb 23 16:18:09 2012 +0000
> @@ -160,13 +160,49 @@ mfn_t __get_gfn_type_access(struct p2m_d
> }
>
> /* For now only perform locking on hap domains */
> - if ( locked && (hap_enabled(p2m->domain)) )
> + locked = locked && hap_enabled(p2m->domain);
> +
> +#ifdef __x86_64__
> +again:
> +#endif
> + if ( locked )
> /* Grab the lock here, don't release until put_gfn */
> gfn_lock(p2m, gfn, 0);
>
> mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order);
>
> #ifdef __x86_64__
> + if ( p2m_is_paging(*t) && (q & P2M_ALLOC)
> + && p2m->domain == current->domain )
> + {
> + if ( locked )
> + gfn_unlock(p2m, gfn, 0);
> +
> + /* Ping the pager */
> + if ( *t == p2m_ram_paging_out || *t == p2m_ram_paged )
> + p2m_mem_paging_populate(p2m->domain, gfn);
> +
> + /* Wait until the pager finishes paging it in */
> + current->arch.mem_paging_gfn = gfn;
> + wait_event(current->arch.mem_paging_wq, ({
> + int done;
> + mfn = p2m->get_entry(p2m, gfn, t, a, 0, page_order);
> + done = (*t != p2m_ram_paging_in);

I assume p2m_mem_paging_populate() will not return until the state is
forwarded to p2m_ram_paging_in. Maybe p2m_is_paging(*t) would make it
more obvious what this check is supposed to do.

> + /* Safety catch: it _should_ be safe to wait here
> + * but if it's not, crash the VM, not the host */
> + if ( in_atomic() )
> + {
> + WARN();
> + domain_crash(p2m->domain);
> + done = 1;
> + }
> + done;
> + }));
> + goto again;
> + }
> +#endif
> +
> +#ifdef __x86_64__
> if ( (q & P2M_UNSHARE) && p2m_is_shared(*t) )
> {
> ASSERT(!p2m_is_nestedp2m(p2m));
> @@ -946,17 +982,17 @@ void p2m_mem_paging_drop_page(struct dom
> * This function needs to be called whenever gfn_to_mfn() returns any of the p2m
> * paging types because the gfn may not be backed by a mfn.
> *
> - * The gfn can be in any of the paging states, but the pager needs only be
> - * notified when the gfn is in the paging-out path (paging_out or paged). This
> - * function may be called more than once from several vcpus. If the vcpu belongs
> - * to the guest, the vcpu must be stopped and the pager notified that the vcpu
> - * was stopped. The pager needs to handle several requests for the same gfn.
> + * The gfn can be in any of the paging states, but the pager needs only
> + * be notified when the gfn is in the paging-out path (paging_out or
> + * paged). This function may be called more than once from several
> + * vcpus. The pager needs to handle several requests for the same gfn.
> *
> - * If the gfn is not in the paging-out path and the vcpu does not belong to the
> - * guest, nothing needs to be done and the function assumes that a request was
> - * already sent to the pager. In this case the caller has to try again until the
> - * gfn is fully paged in again.
> + * If the gfn is not in the paging-out path nothing needs to be done and
> + * the function assumes that a request was already sent to the pager.
> + * In this case the caller has to try again until the gfn is fully paged
> + * in again.
> */
> +
> void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
> {
> struct vcpu *v = current;
> @@ -965,6 +1001,7 @@ void p2m_mem_paging_populate(struct doma
> p2m_access_t a;
> mfn_t mfn;
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> + int send_request = 0;

Is that variable supposed to be used?
Perhaps the feature to fast-forward (or rollback) from
p2m_ram_paging_out to p2m_ram_rw could be a separate patch. My initial
version of this patch did not have a strict requirement for this
feature, if I remember correctly.

> /* We're paging. There should be a ring */
> int rc = mem_event_claim_slot(d, &d->mem_event->paging);
> @@ -987,19 +1024,22 @@ void p2m_mem_paging_populate(struct doma
> /* Evict will fail now, tag this request for pager */
> if ( p2mt == p2m_ram_paging_out )
> req.flags |= MEM_EVENT_FLAG_EVICT_FAIL;
> -
> - set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_paging_in, a);
> + if ( p2mt == p2m_ram_paging_out && mfn_valid(mfn) && v->domain == d )
> + /* Short-cut back to paged-in state (but not for foreign
> + * mappings, or the pager couldn't map it to page it out) */
> + set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
> + paging_mode_log_dirty(d)
> + ? p2m_ram_logdirty : p2m_ram_rw, a);
> + else
> + {
> + set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_ram_paging_in, a);
> + send_request = 1;
> + }
> }
> gfn_unlock(p2m, gfn, 0);
>
> - /* Pause domain if request came from guest and gfn has paging type */
> - if ( p2m_is_paging(p2mt) && v->domain == d )
> - {
> - vcpu_pause_nosync(v);
> - req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> - }
> /* No need to inform pager if the gfn is not in the page-out path */
> - else if ( p2mt != p2m_ram_paging_out && p2mt != p2m_ram_paged )
> + if ( p2mt != p2m_ram_paging_out && p2mt != p2m_ram_paged )
> {
> /* gfn is already on its way back and vcpu is not paused */
> mem_event_cancel_slot(d, &d->mem_event->paging);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 5 of 6] [RFC] x86/mm: use wait queues for mem_paging [ In reply to ]
Hi,

At 14:45 +0100 on 24 Feb (1330094744), Olaf Hering wrote:
> > #ifdef __x86_64__
> > + if ( p2m_is_paging(*t) && (q & P2M_ALLOC)
> > + && p2m->domain == current->domain )
> > + {
> > + if ( locked )
> > + gfn_unlock(p2m, gfn, 0);
> > +
> > + /* Ping the pager */
> > + if ( *t == p2m_ram_paging_out || *t == p2m_ram_paged )
> > + p2m_mem_paging_populate(p2m->domain, gfn);
> > +
> > + /* Wait until the pager finishes paging it in */
> > + current->arch.mem_paging_gfn = gfn;
> > + wait_event(current->arch.mem_paging_wq, ({
> > + int done;
> > + mfn = p2m->get_entry(p2m, gfn, t, a, 0, page_order);
> > + done = (*t != p2m_ram_paging_in);
>
> I assume p2m_mem_paging_populate() will not return until the state is
> forwarded to p2m_ram_paging_in. Maybe p2m_is_paging(*t) would make it
> more obvious what this check is supposed to do.

But it would be wrong. If the type anything other than
p2m_ram_paging_in, then we can't be sure that the pager is working on
unblocking us.

Andres made the same suggestion - clearly this code needs a comment. :)

> > + /* Safety catch: it _should_ be safe to wait here
> > + * but if it's not, crash the VM, not the host */
> > + if ( in_atomic() )
> > + {
> > + WARN();
> > + domain_crash(p2m->domain);
> > + done = 1;
> > + }
> > + done;
> > + }));
> > + goto again;
> > + }
> > +#endif

> > void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
> > {
> > struct vcpu *v = current;
> > @@ -965,6 +1001,7 @@ void p2m_mem_paging_populate(struct doma
> > p2m_access_t a;
> > mfn_t mfn;
> > struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > + int send_request = 0;
>
> Is that variable supposed to be used?

Erk. Clearly something got mangled in the rebase. I'll sort that out.

> Perhaps the feature to fast-forward (or rollback) from
> p2m_ram_paging_out to p2m_ram_rw could be a separate patch. My initial
> version of this patch did not have a strict requirement for this
> feature, if I remember correctly.

Sure, I can split that into a separate patch.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [PATCH 5 of 6] [RFC] x86/mm: use wait queues for mem_paging [ In reply to ]
On Mon, Feb 27, Tim Deegan wrote:

> At 14:45 +0100 on 24 Feb (1330094744), Olaf Hering wrote:
> > > #ifdef __x86_64__
> > > + if ( p2m_is_paging(*t) && (q & P2M_ALLOC)
> > > + && p2m->domain == current->domain )
> > > + {
> > > + if ( locked )
> > > + gfn_unlock(p2m, gfn, 0);
> > > +
> > > + /* Ping the pager */
> > > + if ( *t == p2m_ram_paging_out || *t == p2m_ram_paged )
> > > + p2m_mem_paging_populate(p2m->domain, gfn);
> > > +
> > > + /* Wait until the pager finishes paging it in */
> > > + current->arch.mem_paging_gfn = gfn;
> > > + wait_event(current->arch.mem_paging_wq, ({
> > > + int done;
> > > + mfn = p2m->get_entry(p2m, gfn, t, a, 0, page_order);
> > > + done = (*t != p2m_ram_paging_in);
> >
> > I assume p2m_mem_paging_populate() will not return until the state is
> > forwarded to p2m_ram_paging_in. Maybe p2m_is_paging(*t) would make it
> > more obvious what this check is supposed to do.
>
> But it would be wrong. If the type anything other than
> p2m_ram_paging_in, then we can't be sure that the pager is working on
> unblocking us.

Not really wrong. I think it depends what will happen to the p2mt once
it leaves the paging state and once the condition in wait_event() is
executed again. Now I see what its supposed to mean, it enters
wait_event() with p2m_ram_paging_in and it is supposed to leave once the
type changed to something else. It works because the page-in path has
now only one p2mt, not two like a few weeks ago.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel