Re: [PATCH 1/1] Allow global purge traslation cache (ptc.g) to be disabled - take 2

["Natalie Protasevich" <protasnb@gmail.com>]

On 9/4/07, Bjorn Helgaas <bjorn.helgaas@hp.com> wrote:
> On Monday 03 September 2007 02:06:20 am Natalie Protasevich wrote:
> > This patch allows to disable ptc.g. The code used to be in the kernel, then was removed
> > in 2.4 since the bug that it was fixing has gone away. However, some large system vendors
> > now want this capability available through a means that can be controlled by the platform
> > in the event that there is an issue with either processor or their chipset where global
> > ptc.g is not operational.  They want the mechanism for future platforms to work around
> > such issues. It is also needed for platform makers when they deliberately do not use the
> > global cache purge in their chipset implementation. (For such cases, Intel provided a SAL
> > table entry to specify if ptc.g is allowed and how many).
>
> This is an area prone to hard-to-reproduce and hard-to-debug problems,
> and there's a lot of subtle stuff in this patch.  So I worry about the
> fact that we're adding a noptcg path that will be basically untested,
> compared with the normal path.

I tested it on the tiger before submitting it, so it boots and runs.

>
> > +static inline void
> > +flush_tlb_no_ptcg (unsigned long start, unsigned long end,
> > +                 unsigned long nbits)
> > +{
> > +     extern void smp_send_flush_tlb (void);
> > +     unsigned long saved_tpr = 0;
> > +     unsigned long flags;
> > +     int cpus = num_online_cpus();
>
> This isn't safe with respect to CPU hotplug, is it?  What if a CPU
> goes offline between here and the "wait for other CPUs" loop below?

I will test it with hot plug, good point.

>
> > +     /*
> > +      * Sometimes this is called with interrupts disabled and causes
> > +      * deadlock; to avoid this we enable interrupt and raise the TPR
> > +      * to enable ONLY IPI.
> > +      */
>
> I don't think the comment matches the code.  With TPR, you can mask
> interrupts 0x10-0x1f, 0x10-0x2f, 0x10-0x3f, ..., 0x10-0xef, or 0x10-0xff.
> So you have to leave at least interrupts 0xf0-0xff unmasked, which
> includes IA64_IPI_VECTOR at 0xfe and 15 others.

Hmm, I only see IA64_MCA_WAKEUP_VECTOR and few kinds of IPI above
0xf0. Are those potential ones that you mean?
>
> > +     local_save_flags(flags);
> > +     if (!(flags & IA64_PSR_I)) {
> > +             saved_tpr = ia64_getreg(_IA64_REG_CR_TPR);
> > +             ia64_srlz_d();
>
> Why is this srlz.d needed?
>
> > +             ia64_setreg(_IA64_REG_CR_TPR, saved_tpr);
>
> This just writes back the same value we read above.  It doesn't really
> do anything, does it?

To answer the above two questions I have to look up how TPR works,
can't argue with original code. Maybe Tony or Zou Nan hai can comment
on this.
Same applies to all the following serialization questions. I will do
some research if Intel people won't beat me to answer those (hope they
will :)
Thanks,
--Natalie

>
> > +             ia64_srlz_d();
> > +             local_irq_enable();
> > +     }
> > +
> > +     ia64_global_tlb_flush_rid = ia64_get_rr(start);
> > +     ia64_srlz_d();
>
> Why is this srlz.d needed?

>
> > +     ia64_global_tlb_flush_start = start;
> > +     ia64_global_tlb_flush_end = end;
> > +     ia64_global_tlb_flush_nbits = nbits;
> > +     atomic_set(&ia64_global_tlb_flush_cpu_count, cpus - 1);
> > +     smp_send_flush_tlb();
> > +     /*
> > +      * Purge local TLB entries. ALAT invalidation is done in ia64_leave_kernel.
> > +      */
> > +     do {
> > +             ia64_ptcl(start, nbits<<2);
> > +             start += (1UL << nbits);
> > +     } while (start < end);
> > +
> > +     ia64_srlz_i();                  /* srlz.i implies srlz.d */
> > +
> > +     /*
> > +      * Wait for other CPUs to finish purging entries.
> > +      */
> > +     while (atomic_read(&ia64_global_tlb_flush_cpu_count)) {
> > +             /* Nothing */
> > +     }
> > +
> > +     if (!(flags & IA64_PSR_I)) {
> > +             local_irq_disable();
> > +             ia64_setreg(_IA64_REG_CR_TPR, saved_tpr);
> > +             ia64_srlz_d();
> > +     }
> > +}
> > +
> >  void
> >  ia64_global_tlb_purge (struct mm_struct *mm, unsigned long start,
> >                      unsigned long end, unsigned long nbits)
> > diff -puN arch/ia64/kernel/smp.c~ptcg arch/ia64/kernel/smp.c
> > --- linux-2.6.23-rc5/arch/ia64/kernel/smp.c~ptcg      2007-09-02 23:58:54.000000000 -0700
> > +++ linux-2.6.23-rc5-nataliep/arch/ia64/kernel/smp.c  2007-09-02 23:59:25.000000000 -0700
> > @@ -174,6 +175,48 @@ handle_IPI (int irq, void *dev_id)
> >                               unw_init_running(kdump_cpu_freeze, NULL);
> >                               break;
> >  #endif
> > +
> > +                           case IPI_FLUSH_TLB:
> > +                           {
> > +                             extern unsigned long ia64_global_tlb_flush_start,
> > +                                    ia64_global_tlb_flush_end, ia64_global_tlb_flush_nbits,
> > +                                    ia64_global_tlb_flush_rid;
> > +                             extern atomic_t ia64_global_tlb_flush_cpu_count;
> > +                             unsigned long saved_rid = ia64_get_rr(ia64_global_tlb_flush_start);
> > +                             unsigned long end = ia64_global_tlb_flush_end;
> > +                             unsigned long start = ia64_global_tlb_flush_start;
> > +                             unsigned long nbits = ia64_global_tlb_flush_nbits;
> > +
> > +                             /*
> > +                              * Current CPU may be running with different RID so we need to
> > +                              * reload the RID of flushed address.  Purging the translation
> > +                              * also needs ALAT invalidation; we do not need "invala" here
> > +                              * since it is done in ia64_leave_kernel.
> > +                              */
> > +                             ia64_srlz_d();
>
> Why is this srlz.d needed?
>
> > +                             if (saved_rid != ia64_global_tlb_flush_rid) {
> > +                                     ia64_set_rr(ia64_global_tlb_flush_start, ia64_global_tlb_flush_rid);
> > +                                     ia64_srlz_d();
> > +                             }
> > +
> > +                             do {
> > +                                     /*
> > +                                      * Purge local TLB entries.
> > +                                      */
> > +                                     ia64_ptcl(start, nbits<<2);
> > +                                     start += (1UL << nbits);
> > +                             } while (start < end);
> > +
> > +                             ia64_barrier();
> > +                             ia64_srlz_i();                  /* srlz.i implies srlz.d */
>
> Why are these (barrier & srlz.i) needed?
>
> > +                             if (saved_rid != ia64_global_tlb_flush_rid) {
> > +                                     ia64_set_rr(ia64_global_tlb_flush_start, saved_rid);
> > +                                     ia64_srlz_d();
> > +                             }
> > +                             atomic_dec(&ia64_global_tlb_flush_cpu_count);
> > +                             break;
> > +                           }
> >                             default:
> >                               printk(KERN_CRIT "Unknown IPI on CPU %d: %lu\n", this_cpu, which);
> >                               break;
>