Re: [PATCH v2 09/12] mm,thp: reduce ifdef'ery for THP in generic code

[Vineet Gupta <Vineet.Gupta1@synopsys.com>]

On Friday 09 October 2015 03:24 PM, Kirill A. Shutemov wrote:
> On Tue, Sep 22, 2015 at 04:04:53PM +0530, Vineet Gupta wrote:
>> - pgtable-generic.c: Fold individual #ifdef for each helper into a top
>>   level #ifdef. Makes code more readable
> Makes sense.
>
>> - Per Andrew's suggestion removed the dummy implementations for !THP
>>   in asm-generic/page-table.h to have build time failures vs. runtime.
> I'm not sure it's a good idea. This can lead to unnecessary #ifdefs where
> otherwise call to helper would be eliminated by compiler as dead code.
>
> What about dummy helpers with BUILD_BUG()?

Not really. With this patch, if arch doesn't define __HAVR_ARCH_xyz - we pick the
default implementation. What I changed is if arch defines the __HAVE but doesn't
define the function, then instead of pickign a stub with runtime or buildtime bug
on, we simply fail the build ?

Maybe I can add this to changelog to make it more explicit.

>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  include/asm-generic/pgtable.h | 49 ++++++++++++++++---------------------------
>>  mm/pgtable-generic.c          | 24 +++------------------
>>  2 files changed, 21 insertions(+), 52 deletions(-)
>>
>> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
>> index 29c57b2cb344..2112f4147816 100644
>> --- a/include/asm-generic/pgtable.h
>> +++ b/include/asm-generic/pgtable.h
>> @@ -30,9 +30,12 @@ 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);
>> +
>> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>  #endif
>>  
>>  #ifndef __HAVE_ARCH_PTEP_TEST_AND_CLEAR_YOUNG
>> @@ -64,14 +67,6 @@ static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
>>  		set_pmd_at(vma->vm_mm, address, pmdp, pmd_mkold(pmd));
>>  	return r;
>>  }
>> -#else /* CONFIG_TRANSPARENT_HUGEPAGE */
>> -static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
>> -					    unsigned long address,
>> -					    pmd_t *pmdp)
>> -{
>> -	BUG();
>> -	return 0;
>> -}
>>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>  #endif
>>  
>> @@ -81,8 +76,21 @@ int ptep_clear_flush_young(struct vm_area_struct *vma,
>>  #endif
>>  
>>  #ifndef __HAVE_ARCH_PMDP_CLEAR_YOUNG_FLUSH
>> -int pmdp_clear_flush_young(struct vm_area_struct *vma,
>> -			   unsigned long address, pmd_t *pmdp);
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +extern int pmdp_clear_flush_young(struct vm_area_struct *vma,
>> +				  unsigned long address, pmd_t *pmdp);
>> +#else
>> +/*
>> + * Despite relevant to THP only, this API is called from generic rmap code
>> + * under PageTransHuge(), hence needs a dummy implementation for !THP
>> + */
> Looks like a case I described above. BUILD_BUG_ON() should work fine here.

Indeed BUILD_BUG_ON is better here.

>
>> +static inline int pmdp_clear_flush_young(struct vm_area_struct *vma,
>> +					 unsigned long address, pmd_t *pmdp)
>> +{
>> +	BUG();
>> +	return 0;
>> +}
>> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>  #endif


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>