Re: [PATCH 12/12] um: refactor TLB update handling

["Tiwei Bie" <tiwei.btw@antgroup.com>]

On 4/18/24 5:23 PM, benjamin@sipsolutions.net wrote:
> diff --git a/arch/um/include/asm/mmu.h b/arch/um/include/asm/mmu.h
> index 37eb6e89e79a..bf8da736609c 100644
> --- a/arch/um/include/asm/mmu.h
> +++ b/arch/um/include/asm/mmu.h
> @@ -10,6 +10,10 @@
>  
>  typedef struct mm_context {
>  	struct mm_id id;
> +
> +	/* Address range in need of a TLB sync */
> +	long int sync_tlb_range_from;
> +	long int sync_tlb_range_to;

Why not "unsigned long"?

>  } mm_context_t;
>  
>  extern void __switch_mm(struct mm_id * mm_idp);
> diff --git a/arch/um/include/asm/pgtable.h b/arch/um/include/asm/pgtable.h
> index e1ece21dbe3f..5bb397b65efb 100644
> --- a/arch/um/include/asm/pgtable.h
> +++ b/arch/um/include/asm/pgtable.h
> @@ -244,6 +244,38 @@ static inline void set_pte(pte_t *pteptr, pte_t pteval)
>  
>  #define PFN_PTE_SHIFT		PAGE_SHIFT
>  
> +static inline void um_tlb_mark_sync(struct mm_struct *mm, unsigned long start,
> +				    unsigned long end)
> +{
> +	if (!mm->context.sync_tlb_range_to) {
> +		mm->context.sync_tlb_range_from = start;
> +		mm->context.sync_tlb_range_to = end;
> +	} else {
> +		if (start < mm->context.sync_tlb_range_from)
> +			mm->context.sync_tlb_range_from = start;
> +		if (end > mm->context.sync_tlb_range_to)
> +			mm->context.sync_tlb_range_to = end;
> +	}
> +}

IIUC, in some cases, the range [sync_tlb_range_from, sync_tlb_range_to)
might become very large when merging non-adjacent ranges? Could that
be an issue?

> diff --git a/arch/um/include/asm/tlbflush.h b/arch/um/include/asm/tlbflush.h
> index d7cf82023b74..62816f6f1c91 100644
> --- a/arch/um/include/asm/tlbflush.h
> +++ b/arch/um/include/asm/tlbflush.h
> @@ -9,24 +9,50 @@
>  #include <linux/mm.h>
>  
>  /*
> - * TLB flushing:
> + * In UML, we need to sync the TLB over by using mmap/munmap/mprotect syscalls
> + * from the process handling the MM (which can be the kernel itself).
> + *
> + * To track updates, we can hook into set_ptes and flush_tlb_*. With set_ptes
> + * we catch all PTE transitions where memory that was unusable becomes usable.
> + * While with flush_tlb_* we can track any memory that becomes unusable and
> + * even if a higher layer of the page table was modified.
> + *
> + * So, we simply track updates using both methods and mark the memory area to
> + * be synced later on. The only special case is that flush_tlb_kern_* needs to
> + * be executed immediately as there is no good synchronization point in that
> + * case. In contrast, in the set_ptes case we can wait for the next kernel
> + * segfault before we do the synchornization.
>   *
> - *  - flush_tlb() flushes the current mm struct TLBs
>   *  - flush_tlb_all() flushes all processes TLBs
>   *  - flush_tlb_mm(mm) flushes the specified mm context TLB's
>   *  - flush_tlb_page(vma, vmaddr) flushes one page
> - *  - flush_tlb_kernel_vm() flushes the kernel vm area
>   *  - flush_tlb_range(vma, start, end) flushes a range of pages
> + *  - flush_tlb_kernel_range(start, end) flushes a range of kernel pages
>   */
>  
> +extern int um_tlb_sync(struct mm_struct *mm);
> +
>  extern void flush_tlb_all(void);
>  extern void flush_tlb_mm(struct mm_struct *mm);
> -extern void flush_tlb_range(struct vm_area_struct *vma, unsigned long start, 
> -			    unsigned long end);
> -extern void flush_tlb_page(struct vm_area_struct *vma, unsigned long address);
> -extern void flush_tlb_kernel_vm(void);
> -extern void flush_tlb_kernel_range(unsigned long start, unsigned long end);
> -extern void __flush_tlb_one(unsigned long addr);
> +
> +static void flush_tlb_page(struct vm_area_struct *vma, unsigned long address)
> +{
> +	um_tlb_mark_sync(vma->vm_mm, address, address + PAGE_SIZE);
> +}
> +
> +static void flush_tlb_range(struct vm_area_struct *vma, unsigned long start,
> +			    unsigned long end)
> +{
> +	um_tlb_mark_sync(vma->vm_mm, start, end);
> +}
> +
> +static void flush_tlb_kernel_range(unsigned long start, unsigned long end)
> +{
> +	um_tlb_mark_sync(&init_mm, start, end);
> +
> +	/* Kernel needs to be synced immediately */
> +	um_tlb_sync(&init_mm);
> +}

Nit: this is a header file, these functions should be defined as inline functions.

> diff --git a/arch/um/kernel/tlb.c b/arch/um/kernel/tlb.c
> index c137ff6f84dd..232aa7601d5d 100644
> --- a/arch/um/kernel/tlb.c
> +++ b/arch/um/kernel/tlb.c
[...]
>  
> -void flush_tlb_kernel_range(unsigned long start, unsigned long end)
> -{
> -	flush_tlb_kernel_range_common(start, end);
> -}
> -
> -void flush_tlb_kernel_vm(void)
> -{
> -	flush_tlb_kernel_range_common(start_vm, end_vm);
> -}

The build breaks with this change, as there is still a call to
flush_tlb_kernel_vm() in ubd.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/um/drivers/ubd_kern.c?id=fb5d1d389c9e78d68f1f71f926d6251017579f5b#n774

Regards,
Tiwei