Re: [PATCH v5 2/5] mm: Add generic set_ptes()

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Wed, Feb 08, 2023 at 06:14:48PM +0000, Matthew Wilcox wrote:
> On Wed, Feb 08, 2023 at 11:38:48AM +0300, Kirill A. Shutemov wrote:
> > > +static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> > > +		pte_t *ptep, pte_t pte, unsigned int nr)
> > > +{
> > > +	for (;;) {
> > > +		set_pte_at(mm, addr, ptep, pte);
> > > +		if (--nr == 0)
> > > +			break;
> > 
> > Maybe do { ... } while (--nr); instead of the for()?
> 
> Seems unnecessary to do the extra calculation to advance pte if
> we're not going to use it?

It should be transparent to compiler, but okay.

> 
> > > +		ptep++;
> > > +		addr += PAGE_SIZE;
> > > +		/* This works for x86.  Check how PTEs are encoded */
> > > +		pte = __pte(pte_val(pte) + PAGE_SIZE);
> > 
> > Looks like it deserves own helper. Something like
> > 
> > 		pte = pte_next(pte);
> 
> Maybe, but then it needs to be added to each arch, which I was trying
> to avoid.  That said, I had another idea which I like better; more
> patches coming soon.

Generic pte_next() that does the "+ PAGE_SIZE" trick should be fine. Arch
code can override it if bit placement is different, instead of rewriting
whole set_ptes()'

> Here's a sneak preview:
> 
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -1019,13 +1019,22 @@ static inline pud_t native_local_pudp_get_and_clear(pud_
> t *pudp)
>         return res;
>  }
> 
> -static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
> -                             pte_t *ptep, pte_t pte)
> +static inline void set_ptes(struct mm_struct *mm, unsigned long addr,
> +                             pte_t *ptep, pte_t pte, unsigned int nr)
>  {
> -       page_table_check_ptes_set(mm, addr, ptep, pte, 1);
> -       set_pte(ptep, pte);
> +       page_table_check_ptes_set(mm, addr, ptep, pte, nr);
> +
> +       for (;;) {
> +               set_pte(ptep, pte);
> +               if (--nr == 0)
> +                       break;
> +               ptep++;
> +               pte = __pte(pte_val(pte) + PAGE_SIZE);
> +       }

	for(; nr; nr--, ptep++, pte = pte_next(pte))
		set_pte(ptep, pte);

Hm?

>  }
> 
> +#define set_pte_at(mm, addr, ptep, pte) set_ptes(mm, addr, ptep, pte, 1)
> +
>  static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
>                               pmd_t *pmdp, pmd_t pmd)
>  {
> 
> 
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov