Re: [PATCH] common/config: Always create RESULT_BASE

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sun, Jan 06, 2019 at 09:47:34AM -0800, Darrick J. Wong wrote:
> On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> > On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> > > 
> > > 
> > > On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > > > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > > > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > > > to only create the directory only if this variable is not explicitly set
> > > > by the user. Fix this by ensuring RESULT_BASE is always created.
> > > > 
> > > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > > 
> > > Eryu,
> > > 
> > > I think Johannes' commit should actually be reverted. Currently
> > > get_next_config is called at the beginning of the section code _AFTER_
> > > _init_kmemleak so it's not really fixing the problem that Johannes
> > > described. So care to revert his commit ?
> > 
> > Yes, the original commit should be reverted. Using RESULT_BASE in
> > the way that init_kmemleak is using it is fundamentally broken.
> > Using RESULTS_BASE is incorrect - harness run state is supposed to
> > be stored in the section-aware RESULTS_DIR (e.g. see
> > _require_scratch()) which is constant and always exists for each
> > section that is run.
> > 
> > The code was architectected this way because section definitions can
> > change RESULT_BASE, and that means the RESULT_BASE defined when
> > init_kmemleak() is called may not point to the same location as when
> > check_memleak() is called. Hence check_memleak will never run if a
> > section definition changes RESULT_BASE.
> > 
> > IOWs, the init_kmemleak() call needs to be done inside the
> > section iteration loop, after the section config has been parsed and
> > we've determined if the the section will be run. This is where the
> > original code created RESULT_BASE if it didn't exist. Further, all
> > the section run state is then stored in RESULTS_DIR, which is
> > created from the current RESULT_BASE (e.g. see _require_scratch,
> > check_dmesg, etc).
> > 
> > Hence init_kmemleak() and check_kmemleak() should have a scope
> > inside a valid RESULT_DIR path. And once this is done, you can then
> 
> However, the scope of RESULT_DIR is per-test, not per-section.  Either
> we'd have to hoist the creation of RESULT_DIR above the "for seq in
> $list..." or engage in some egregious hacks to get this set up before
> we loop through the tests.
> 
> The other problem with kmemleak is that it takes a while for the scanner
> to notice leaks (hence all the "read report twice" crap code) which
> means we'd really don't want to be cycling it on and off repeatedly,
> because that causes it to lose leaks periodically.  It works best when
> you turn it on once and sample every now and then.

Ahh, we don't cycle the background scanner anymore, we use the
_init_kmemleak method to disable background scanning and discard all the
leaks that happened before the user even ran ./check.  So that only
needs to run once at the very beginning, which is how it behaves now.

As for _check_kmemleak, we actually need it to run after ever test, if
nothing else to clean out corruption reports.  I can make it discard the
results if USE_KMEMLEAK=no or CONFIG_DEBUG_KMEMLEAK=n.  I will also
change it to drop the report in the $RESULT_DIR instead of $RESULT_BASE,
as discussed.

--D

> Frankly, the kmemleak mechanism is just grody and unreliable enough that
> I'm tempted to ask Eryu to rip it out entirely.  But, I'll give one go
> at fixing it, since I wrote it...
> 
> --D
> 
> > add a USE_KMEMLEAK section variable to turn kmemleak detection on
> > and off via the config file on a per-section basis.....
> > 
> > So, IMO, the correct thing to do here is revert the original broken
> > change and fix the kmemleak infrastructure to be properly aware of
> > config sections.
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com