Re: [Xen-devel] [PATCH v2 1/2] xen/balloon: set a mapping for ballooned out pages

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2013-07-23 at 16:03 +0100, David Vrabel wrote:
> On 23/07/13 15:27, Ian Campbell wrote:
> > On Tue, 2013-07-23 at 10:58 +0100, David Vrabel wrote:
> >> On 22/07/13 18:32, Ian Campbell wrote:
> >>> On Mon, 2013-07-22 at 18:26 +0100, David Vrabel wrote:
> >>>> On 22/07/13 18:22, Ian Campbell wrote:
> >>>>> On Mon, 2013-07-22 at 17:51 +0100, David Vrabel wrote:
> >>>>>> On 22/07/13 17:28, Stefano Stabellini wrote:
> >>>>>>>
> >>>>>>>  #ifdef CONFIG_HIGHMEM
> >>>>>>>  #define inc_totalhigh_pages() (totalhigh_pages++)
> >>>>>>> @@ -423,7 +426,8 @@ static enum bp_state 
> >>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
> >>>>>>>               if (xen_pv_domain() && !PageHighMem(page)) {
> >>>>>>>                       ret = HYPERVISOR_update_va_mapping(
> >>>>>>>                               (unsigned long)__va(pfn << PAGE_SHIFT),
> >>>>>>> -                             __pte_ma(0), 0);
> >>>>>>> +                             
> >>>>>>> pfn_pte(page_to_pfn(get_balloon_trade_page()),
> >>>>>>> +                                     PAGE_KERNEL_RO), 0);
> >>>>>>
> >>>>>> Preemption needs to be disabled while using the trade page, see
> >>>>>> suggestion below.
> >>>>>
> >>>>> Hopefully you mean just when setting up/manipulating it?
> >>>>
> >>>> Yes, sorry.
> >>>>
> >>>> get_...()
> >>>> update_va_mapping()
> >>>> put_...()
> >>>
> >>> I can see why it would matter in the unmap_and_replace+fixup case (since
> >>> you need them to happen "atomically") but why in this case? We don't
> >>> actually care which of the trade pages gets used for this purpose, so
> >>> even if we happen to get preempted and change CPU it doesn't really
> >>> matter.
> >>
> >> If a trade page from another CPU is used, it may concurrently have it's
> >> MFN cleared by a unmap_and_replace call on the other CPU.
> > 
> > The call on the other CPU clears the virtual address, not the MFN, so
> > long as we have the MFN in our hand we are OK, I think? Nothing updates
> > the m2p or p2m does it?
> 
> Yes, I think you're correct here.

Oh good!

That said if other callers do need the locking it would be better to use
it consistently everywhere I suppose, plus it means we have to think
less hard about correctness ;-)

> But, does this need to be mfn_pte(page_to_mfn(get_balloon_trade_page())?
>  The pte parameter to update_va_mapping() needs to contains the machine
> address, right?

I think pfn_pte(page_to_pfn(...)) and mfn_pte(page_to_mfn(...)) are
equivalent, just the p2m translation happens in pfn_pte vs page_to_mfn.
The pfn version is the more normal "generic" thing whereas the mfn
versions are Xen special cases used for special things (like mfns with
no equivalent pfn, 1:1 mappings and the like).

Ian.


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