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

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

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


> >  @@ -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.
-- 
yamahata

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