Re: [Xen-ia64-devel] RFC: ptc.ga implementation for SMP-g

[Alex Williamson <alex.williamson@xxxxxx>]

On Mon, 2006-04-03 at 14:38 +0100, Tristan Gingold wrote:
> Hi,
> 
> after the comments, here is my updated patch for ptc.ga
> Please comment it.
> 
> With this patch, the page_flags are always written atomically.  Ptc only 
> clear 
> it.  This eliminates itc and ptc conflicts.
> 
> The other conflict is use.  This is within ia64_page_fault, between 
> vcpu_translate and vcpu_itc_no_srlz.  This part of code is protected by a 
> flag + counter: At entry the flag is set and the counter increment, at exit 
> the flag is reset.  Ptc.ga waits if the flag is set and retries if the 
> counter has changes. 

Hi Tristan,

   Is there any way a nested page fault could double increment tlb_inuse
(ie. where you might hit that BUG_ON)?  This locking looks a lot like a
seqlock.  Could the bit in ia64_do_page_fault() be replaced by:

write_seqcount_begin(&current->arch.tlb_inuse);
fault = vcpu_translate(...
if (fault == IA64_NO_FAULT) {
        ...
        write_seqcount_end(&current->arch.tlb_inuse);
        return;
}
write_seqcount_end(&current->arch.tlb_inuse);

The other user could then be:

do {
        seq = read_seqcount_begin(&current->arch.tlb_inuse);
        vcpu_purge_tr_entry(...
        vcpu_purge_tr_entry(...
while (read_seqcount_retry(&current->arch.tlb_inuse, seq);

Hopefully we can remove the cpu_relax() here, if not, the locking is
probably racy anyway.  We can't really implement the BUG_ON legitmately
with a seqlock since the sequence number is meant to be opaque.  If we
can't guarantee that we only have one "writer" per lock at a time, we
could use the write_seqlock/sequnlock(), but obviously the extra
spinlock adds overhead and a nested fault would still be difficult to
deal with (and maybe deadlock w/o careful use of write_tryseqlock()).

   BTW, lkml strongly discourages setting variables inside tests like
while ((count = PSCBX(vcpu, tlb_inuse)) & 1).  The preferred mechanism
is to separate them into two statements:

        count = PSCBX(vcpu, tlb_inuse);
        while (count & 1)

Thanks,

        Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab


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