Re: [PATCH 04/11] ARCv2: mm: THP support

Re: [PATCH 04/11] ARCv2: mm: THP support

[Kirill A. Shutemov]

On Thu, Aug 27, 2015 at 02:33:07PM +0530, Vineet Gupta wrote:
> +pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
> +{
> +	struct list_head *lh;
> +	pgtable_t pgtable;
> +	pte_t *ptep;
> +
> +	assert_spin_locked(&mm->page_table_lock);
> +
> +	pgtable = pmd_huge_pte(mm, pmdp);
> +	lh = (struct list_head *) pgtable;
> +	if (list_empty(lh))
> +		pmd_huge_pte(mm, pmdp) = (pgtable_t) NULL;
> +	else {
> +		pmd_huge_pte(mm, pmdp) = (pgtable_t) lh->next;
> +		list_del(lh);
> +	}

Side question: why pgtable_t is unsigned long on ARC and not struct page *
or pte_t *, like on other archs? We could avoid these casts.

-- 
 Kirill A. Shutemov

Re: [PATCH 04/11] ARCv2: mm: THP support

[Vineet Gupta]

On Thursday 27 August 2015 09:02 PM, Kirill A. Shutemov wrote:
> On Thu, Aug 27, 2015 at 02:33:07PM +0530, Vineet Gupta wrote:
>> > +pgtable_t pgtable_trans_huge_withdraw(struct mm_struct *mm, pmd_t *pmdp)
>> > +{
>> > +	struct list_head *lh;
>> > +	pgtable_t pgtable;
>> > +	pte_t *ptep;
>> > +
>> > +	assert_spin_locked(&mm->page_table_lock);
>> > +
>> > +	pgtable = pmd_huge_pte(mm, pmdp);
>> > +	lh = (struct list_head *) pgtable;
>> > +	if (list_empty(lh))
>> > +		pmd_huge_pte(mm, pmdp) = (pgtable_t) NULL;
>> > +	else {
>> > +		pmd_huge_pte(mm, pmdp) = (pgtable_t) lh->next;
>> > +		list_del(lh);
>> > +	}
> Side question: why pgtable_t is unsigned long on ARC and not struct page *
> or pte_t *, like on other archs? We could avoid these casts.

This goes back how I did this for ARC long back to avoid page_address() calls in
general case. e.g. pte_alloc_one(), pmd_populate(), pte_free()... all needed to
convert struct page to unsigned long. It was micro-optimization of sorts, but
served us well.

I could perhaps see try making it pte *, that will certainly remove a bunch of
other casts as well.

-Vineet

Re: [PATCH 08/11] mm,thp: reduce ifdef'ery for THP in generic code

[Andrew Morton]

On Thu, 27 Aug 2015 14:33:11 +0530 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> This is purely cosmetic, just makes code more readable
> 
> ...
>
> --- a/include/asm-generic/pgtable.h
> +++ b/include/asm-generic/pgtable.h
> @@ -30,9 +30,20 @@ extern int ptep_set_access_flags(struct vm_area_struct *vma,
>  #endif
>  
>  #ifndef __HAVE_ARCH_PMDP_SET_ACCESS_FLAGS
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  extern int pmdp_set_access_flags(struct vm_area_struct *vma,
>  				 unsigned long address, pmd_t *pmdp,
>  				 pmd_t entry, int dirty);
> +#else /* CONFIG_TRANSPARENT_HUGEPAGE */
> +static inline int pmdp_set_access_flags(struct vm_area_struct *vma,
> +					unsigned long address, pmd_t *pmdp,
> +					pmd_t entry, int dirty)
> +{
> +	BUG();
> +	return 0;
> +}

Is it possible to simply leave this undefined?  So the kernel fails at
link time?

> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c

Good heavens that file is a mess.  Your patch does improve it.

Re: [PATCH 09/11] mm,thp: introduce flush_pmd_tlb_range

[Andrew Morton]

On Thu, 27 Aug 2015 14:33:12 +0530 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> --- a/mm/pgtable-generic.c
> +++ b/mm/pgtable-generic.c
> @@ -84,6 +84,19 @@ pte_t ptep_clear_flush(struct vm_area_struct *vma, unsigned long address,
>  
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  
> +#ifndef __HAVE_ARCH_FLUSH_PMD_TLB_RANGE
> +
> +/*
> + * ARCHes with special requirements for evicting THP backing TLB entries can
> + * implement this. Otherwise also, it can help optimizing thp flush operation.
> + * flush_tlb_range() can have optimization to nuke the entire TLB if flush span
> + * is greater than a threashhold, which will likely be true for a single
> + * huge page.
> + * e.g. see arch/arc: flush_pmd_tlb_range
> + */
> +#define flush_pmd_tlb_range(vma, addr, end)	flush_tlb_range(vma, addr, end)
> +#endif

Did you consider using a __weak function here?

Re: [PATCH 00/11] THP support for ARC

[Andrew Morton]

On Thu, 27 Aug 2015 14:33:03 +0530 Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:

> This series brings THP support to ARC. It also introduces an optional new
> thp hook for arches to possibly optimize the TLB flush in thp regime.

The mm/ changes look OK to me.  Please maintain them in the arc tree,
merge them upstream at the same time.