Re: Re: [PATCH v4 10/11] mm/damon: Add kunit tests

Re: Re: [PATCH v4 10/11] mm/damon: Add kunit tests

[SeongJae Park]

On Fri, 14 Feb 2020 00:56:50 +0800 kbuild test robot <lkp@intel.com> wrote:

> [-- Attachment #1: Type: text/plain, Size: 3435 bytes --]
> 
> Hi,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on d5226fa6dbae0569ee43ecfc08bdcd6770fc4755]
> 
> url:    https://github.com/0day-ci/linux/commits/sjpark-amazon-com/Introduce-Data-Access-MONitor-DAMON/20200213-003254
> base:    d5226fa6dbae0569ee43ecfc08bdcd6770fc4755
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-7 (Debian 7.5.0-4) 7.5.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    In file included from mm/damon.c:19:0:
> >> include/linux/module.h:131:42: error: redefinition of '__inittest'
>      static inline initcall_t __maybe_unused __inittest(void)  \
>                                              ^
>    include/linux/module.h:124:28: note: in expansion of macro 'module_init'
>     #define late_initcall(fn)  module_init(fn)
>                                ^~~~~~~~~~~
>    include/kunit/test.h:224:2: note: in expansion of macro 'late_initcall'
>      late_initcall(kunit_suite_init##suite)
>      ^~~~~~~~~~~~~
>    mm/damon-test.h:600:1: note: in expansion of macro 'kunit_test_suite'
>     kunit_test_suite(damon_test_suite);
>     ^~~~~~~~~~~~~~~~
>    include/linux/module.h:131:42: note: previous definition of '__inittest' was here
>      static inline initcall_t __maybe_unused __inittest(void)  \
>                                              ^
>    mm/damon.c:1406:1: note: in expansion of macro 'module_init'
>     module_init(damon_init);
>     ^~~~~~~~~~~
> >> include/linux/module.h:133:6: error: redefinition of 'init_module'
>      int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
>          ^
>    include/linux/module.h:124:28: note: in expansion of macro 'module_init'
>     #define late_initcall(fn)  module_init(fn)
>                                ^~~~~~~~~~~
>    include/kunit/test.h:224:2: note: in expansion of macro 'late_initcall'
>      late_initcall(kunit_suite_init##suite)
>      ^~~~~~~~~~~~~
>    mm/damon-test.h:600:1: note: in expansion of macro 'kunit_test_suite'
>     kunit_test_suite(damon_test_suite);
>     ^~~~~~~~~~~~~~~~
>    include/linux/module.h:133:6: note: previous definition of 'init_module' was here
>      int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
>          ^
>    mm/damon.c:1406:1: note: in expansion of macro 'module_init'
>     module_init(damon_init);
>     ^~~~~~~~~~~
> 
> vim +/__inittest +131 include/linux/module.h
> 
> 0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  128  
> 0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  129  /* Each module must use one module_init(). */
> 0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  130  #define module_init(initfn)					\
> 1f318a8bafcfba9 Arnd Bergmann  2017-02-01 @131  	static inline initcall_t __maybe_unused __inittest(void)		\
> 0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  132  	{ return initfn; }					\
> a6e60d84989fa0e Miguel Ojeda   2019-01-19 @133  	int init_module(void) __copy(initfn) __attribute__((alias(#initfn)));
> 0fd972a7d91d6e1 Paul Gortmaker 2015-05-01  134  
> 
> :::::: The code at line 131 was first introduced by commit
> :::::: 1f318a8bafcfba9f0d623f4870c4e890fd22e659 modules: mark __inittest/__exittest as __maybe_unused
> 
> :::::: TO: Arnd Bergmann <arnd@arndb.de>
> :::::: CC: Jessica Yu <jeyu@redhat.com>

Thank you for finding yet another problem!  The problem is reproducible if
`CONFIG_DAMON=m` but `CONFIG_DAMON_KUNIT_TEST=y`.  Will fix this problem by
simply adjusting the dependency of the tests as below.  It will avoid the build
of the test code when DAMON is configured to be built as a module::

    diff --git a/mm/Kconfig b/mm/Kconfig
    index b279ab9c78d0..1a745ce0cbcb 100644
    --- a/mm/Kconfig
    +++ b/mm/Kconfig
    @@ -753,7 +753,7 @@ config DAMON
    
     config DAMON_KUNIT_TEST
            bool "Test for damon"
    -       depends on DAMON && KUNIT
    +       depends on DAMON=y && KUNIT
            help
              This builds the DAMON Kunit test suite.

I think this is fair enough as KUNIT is not exporting its main functions to
modules and thus cannot be used by modules anyway.


Thanks,
SeongJae Park

> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 
> [-- Attachment #2: .config.gz --]
> [-- Type: application/gzip, Size: 71807 bytes --]

Re: Re: [PATCH v4 10/11] mm/damon: Add kunit tests

[SeongJae Park]

On Fri, 14 Feb 2020 20:07:47 -0800 Randy Dunlap <rdunlap@infradead.org> wrote:

> On 2/10/20 6:53 AM, sjpark@amazon.com wrote:
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index 387d469f40ec..b279ab9c78d0 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -751,4 +751,15 @@ config DAMON
> >  	  be 1) accurate enough to be useful for performance-centric domains,
> >  	  and 2) sufficiently light-weight so that it can be applied online.
> >  
> > +config DAMON_KUNIT_TEST
> > +	bool "Test for damon"
> 
> s/bool/tristate/ ?

Thank you for this comment!

It seems Kunit does not support module build, as its core functions are not
exported to modules.  That said, as this might be confusing and even could
cause a build failure with some configuration combinations[1], I will change
this dependency to `DAMON=y && KUNIT` in next spin.

[1] https://lore.kernel.org/linux-mm/20200214111907.7017-1-sjpark@amazon.com/

Thanks,
SeongJae Park

> 
> > +	depends on DAMON && KUNIT
> > +	help
> > +	  This builds the DAMON Kunit test suite.
> > +
> > +	  For more information on KUnit and unit tests in general, please refer
> > +	  to the KUnit documentation.
> > +
> > +	  If unsure, say N.
> > +
> >  endmenu
> 
> 
> -- 
> ~Randy
>