On Tue, 20 Jun 2017, Andy Lutomirski wrote:
> struct flush_tlb_info {
> + /*
> + * We support several kinds of flushes.
> + *
> + * - Fully flush a single mm. flush_mm will be set, flush_end will be
flush_mm is the *mm member in the struct, right? You might rename that as a
preparatory step so comments and implementation match.
> + * TLB_FLUSH_ALL, and new_tlb_gen will be the tlb_gen to which the
> + * IPI sender is trying to catch us up.
> + *
> + * - Partially flush a single mm. flush_mm will be set, flush_start
> + * and flush_end will indicate the range, and new_tlb_gen will be
> + * set such that the changes between generation new_tlb_gen-1 and
> + * new_tlb_gen are entirely contained in the indicated range.
> + *
> + * - Fully flush all mms whose tlb_gens have been updated. flush_mm
> + * will be NULL, flush_end will be TLB_FLUSH_ALL, and new_tlb_gen
> + * will be zero.
> + */
> struct mm_struct *mm;
> unsigned long start;
> unsigned long end;
> + u64 new_tlb_gen;
Nit. While at it could you please make that struct tabular aligned as we
usually do in x86?
> static void flush_tlb_func_common(const struct flush_tlb_info *f,
> bool local, enum tlb_flush_reason reason)
> {
> + struct mm_struct *loaded_mm = this_cpu_read(cpu_tlbstate.loaded_mm);
> +
> + /*
> + * Our memory ordering requirement is that any TLB fills that
> + * happen after we flush the TLB are ordered after we read
> + * active_mm's tlb_gen. We don't need any explicit barrier
> + * because all x86 flush operations are serializing and the
> + * atomic64_read operation won't be reordered by the compiler.
> + */
Can you please move the comment above the loaded_mm assignment?
> + u64 mm_tlb_gen = atomic64_read(&loaded_mm->context.tlb_gen);
> + u64 local_tlb_gen = this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen);
> +
> /* This code cannot presently handle being reentered. */
> VM_WARN_ON(!irqs_disabled());
>
> + VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
> + loaded_mm->context.ctx_id);
> +
> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
> + /*
> + * leave_mm() is adequate to handle any type of flush, and
> + * we would prefer not to receive further IPIs.
While I know what you mean, it might be useful to have a more elaborate
explanation why this prevents new IPIs.
> + */
> leave_mm(smp_processor_id());
> return;
> }
>
> - if (f->end == TLB_FLUSH_ALL) {
> - local_flush_tlb();
> - if (local)
> - count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
> - trace_tlb_flush(reason, TLB_FLUSH_ALL);
> - } else {
> + if (local_tlb_gen == mm_tlb_gen) {
> + /*
> + * There's nothing to do: we're already up to date. This can
> + * happen if two concurrent flushes happen -- the first IPI to
> + * be handled can catch us all the way up, leaving no work for
> + * the second IPI to be handled.
That not restricted to IPIs, right? A local flush / IPI combo can do that
as well.
Other than those nits;
Reviewed-by: Thomas Gleixner <t...@linutronix.de>
