On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
> +/**
> + * struct wp_walk - Private struct for pagetable walk callbacks
> + * @range: Range for mmu notifiers
> + * @tlbflush_start: Address of first modified pte
> + * @tlbflush_end: Address of last modified pte + 1
> + * @total: Total number of modified ptes
> + */
> +struct wp_walk {
> + struct mmu_notifier_range range;
> + unsigned long tlbflush_start;
> + unsigned long tlbflush_end;
> + unsigned long total;
> +};
> +
> +/**
> + * wp_pte - Write-protect a pte
> + * @pte: Pointer to the pte
> + * @addr: The virtual page address
> + * @walk: pagetable walk callback argument
> + *
> + * The function write-protects a pte and records the range in
> + * virtual address space of touched ptes for efficient range TLB flushes.
> + */
> +static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
> + struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> + pte_t ptent = *pte;
> +
> + if (pte_write(ptent)) {
> + pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
> +
> + ptent = pte_wrprotect(old_pte);
> + ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);
> + wpwalk->total++;
> + wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
> + wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
> + addr + PAGE_SIZE);
> + }
> +
> + return 0;
> +}
> +/*
> + * wp_clean_pre_vma - The pagewalk pre_vma callback.
> + *
> + * The pre_vma callback performs the cache flush, stages the tlb flush
> + * and calls the necessary mmu notifiers.
> + */
> +static int wp_clean_pre_vma(unsigned long start, unsigned long end,
> + struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> +
> + wpwalk->tlbflush_start = end;
> + wpwalk->tlbflush_end = start;
> +
> + mmu_notifier_range_init(&wpwalk->range, MMU_NOTIFY_PROTECTION_PAGE, 0,
> + walk->vma, walk->mm, start, end);
> + mmu_notifier_invalidate_range_start(&wpwalk->range);
> + flush_cache_range(walk->vma, start, end);
> +
> + /*
> + * We're not using tlb_gather_mmu() since typically
> + * only a small subrange of PTEs are affected, whereas
> + * tlb_gather_mmu() records the full range.
> + */
> + inc_tlb_flush_pending(walk->mm);
> +
> + return 0;
> +}
> +
> +/*
> + * wp_clean_post_vma - The pagewalk post_vma callback.
> + *
> + * The post_vma callback performs the tlb flush and calls necessary mmu
> + * notifiers.
> + */
> +static void wp_clean_post_vma(struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> +
> + if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
> + flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
> + wpwalk->tlbflush_end);
> +
> + mmu_notifier_invalidate_range_end(&wpwalk->range);
> + dec_tlb_flush_pending(walk->mm);
> +}
> +/**
> + * wp_shared_mapping_range - Write-protect all ptes in an address space range
> + * @mapping: The address_space we want to write protect
> + * @first_index: The first page offset in the range
> + * @nr: Number of incremental page offsets to cover
> + *
> + * Note: This function currently skips transhuge page-table entries, since
> + * it's intended for dirty-tracking on the PTE level. It will warn on
> + * encountering transhuge write-enabled entries, though, and can easily be
> + * extended to handle them as well.
> + *
> + * Return: The number of ptes actually write-protected. Note that
> + * already write-protected ptes are not counted.
> + */
> +unsigned long wp_shared_mapping_range(struct address_space *mapping,
> + pgoff_t first_index, pgoff_t nr)
> +{
> + struct wp_walk wpwalk = { .total = 0 };
> +
> + i_mmap_lock_read(mapping);
> + WARN_ON(walk_page_mapping(mapping, first_index, nr, &wp_walk_ops,
> + &wpwalk));
> + i_mmap_unlock_read(mapping);
> +
> + return wpwalk.total;
> +}
That's a read lock, this means there's concurrency to self. What happens
if someone does two concurrent wp_shared_mapping_range() on the same
mapping?
The thing is, because of pte_wrprotect() the iteration that starts last
will see a smaller pte_write range, if it completes first and does
flush_tlb_range(), it will only flush a partial range.
This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
not using mm_tlb_flush_nested() to detect the situation and do a bigger
flush.
Or if you're not needing that, then I'm missing why.
