Re: [PATCH 5 of 6] [RFC] x86/mm: use wait queues for mem_paging

[Olaf Hering <olaf@aepfle.de>]

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);