On Wed, Jun 21, 2017 at 1:32 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 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.
The comment is outdated. Fixed now.
>
>> + * 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?
Sure.
>
>> 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?
I'll move it above the function entirely. It's more of a general
comment about how the function works than any particular part of the
function.
>
>> + 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.
Added, although it just gets deleted again later in the series.
>
>> + */
>> 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.
Indeed. Comment fixed.
>
> Other than those nits;
>
> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
