Re: [Xen-ia64-devel] [PATCH 2/7][SMP] add volatile to p2m table pte entry

[Al Stone <ahs3@xxxxxxxxx>]

On Fri, 2006-06-16 at 11:00 +0900, Isaku Yamahata wrote:
> On Thu, Jun 15, 2006 at 01:14:06PM -0600, Al Stone wrote:
> 
> > In all of the functions above, it appears that the return value of
> > a function (pte_offset_map()) is being returned as a volatile result
> > from each of the functions.  Is that really needed?  I'm not sure
> > it helps in this case, but I could be wrong.
> 
> It seems that you are confusing
> volatile pte_t* (a pointer to volatile pte_t) with
> pte_t* volatile (a volatile pointer to pte_t).

Argh.  Yes, you are correct.

> 
> > >  @@ -986,21 +1034,42 @@ destroy_grant_host_mapping(unsigned long
> > >       }
> > >   
> > >       pte = lookup_noalloc_domain_pte(d, gpaddr);
> > >  -    if (pte == NULL || !pte_present(*pte) || pte_pfn(*pte) != mfn)
> > >  +    if (pte == NULL) {
> > >  +        DPRINTK("%s: gpaddr 0x%lx mfn 0x%lx\n", __func__, gpaddr, mfn);
> > >           return GNTST_general_error;
> > >  -
> > >  -    // update pte
> > >  -    old_pte = ptep_get_and_clear(&d->arch.mm, gpaddr, pte);
> > >  -    if (pte_present(old_pte)) {
> > >  -        old_mfn = pte_pfn(old_pte);
> > >  -    } else {
> > >  +    }
> > >  +
> > >  + again:
> > >  +    cur_arflags = pte_val(*pte) & ~_PAGE_PPN_MASK;
> > >  +    cur_pte = pfn_pte(mfn, __pgprot(cur_arflags));
> > >  +    if (!pte_present(cur_pte)) {
> > >  +        DPRINTK("%s: gpaddr 0x%lx mfn 0x%lx cur_pte 0x%lx\n",
> > >  +                __func__, gpaddr, mfn, pte_val(cur_pte));
> > >           return GNTST_general_error;
> > >       }
> > >  -    domain_page_flush(d, gpaddr, old_mfn, INVALID_MFN);
> > >  -
> > >  -    old_page = mfn_to_page(old_mfn);
> > >  -    BUG_ON(page_get_owner(old_page) == 
> > > d);//try_to_clear_PGC_allocate(d, page) is not needed.
> > >  -    put_page(old_page);
> > >  +    new_pte = __pte(0);
> > >  +
> > >  +    old_pte = ptep_cmpxchg_rel(&d->arch.mm, gpaddr, pte, cur_pte, 
> > > new_pte);
> > >  +    if (unlikely(!pte_present(old_pte))) {
> > >  +        DPRINTK("%s: gpaddr 0x%lx mfn 0x%lx cur_pte 0x%lx old_pte 
> > > 0x%lx\n",
> > >  +                __func__, gpaddr, mfn, pte_val(cur_pte), 
> > > pte_val(old_pte));
> > >  +        return GNTST_general_error;
> > >  +    }
> > >  +    if (unlikely(pte_val(cur_pte) != pte_val(old_pte))) {
> > >  +        if (pte_pfn(old_pte) == mfn) {
> > >  +            goto again;
> > 
> > Maybe I'm just being paranoid, but is there *any* chance this goto loop
> > will not terminate?
> 
> Yes there is.
> If there are more than two vcpus and enough physical cpus, and
> other vcpus keep chainging the entry, the goto loop won't terminate.
> I think it is very unlikey in practice.

Thanks for the clarification.  I'll have to think about this a bit.
Guaranteeing the loop terminates could be a lot more expensive than
the risk of not terminating.  Like you say, the scenario is unlikely.

-- 
Ciao,
al
----------------------------------------------------------------------
Al Stone                                      Alter Ego:
Open Source and Linux R&D                     Debian Developer
Hewlett-Packard Company                       http://www.debian.org
E-mail: ahs3@xxxxxxxxx                        ahs3@xxxxxxxxxx
----------------------------------------------------------------------


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel