[3/4] common: fix kmemleak to work with sections
diff mbox series

Message ID 154877864538.9277.6249607598508212314.stgit@magnolia
State Accepted
Headers show
Series
  • [1/4] xfs/093: make sure the scratch directory still exists after repair
Related show

Commit Message

Darrick J. Wong Jan. 29, 2019, 4:17 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the kmemleak code to work correctly with sections.  This
requires changing the report location to use RESULT_DIR instead of
RESULT_BASE, and clarifying which functions get used when.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 check     |    4 ++--
 common/rc |   32 ++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 12 deletions(-)

Comments

Eryu Guan Feb. 3, 2019, 9:10 a.m. UTC | #1
On Tue, Jan 29, 2019 at 08:17:25AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the kmemleak code to work correctly with sections.  This

Thanks for the fix!

> requires changing the report location to use RESULT_DIR instead of
> RESULT_BASE, and clarifying which functions get used when.

But I didn't see any RESULT_DIR related changes in this patch.

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  check     |    4 ++--
>  common/rc |   32 ++++++++++++++++++++++----------
>  2 files changed, 24 insertions(+), 12 deletions(-)
> 
> 
> diff --git a/check b/check
> index c0eee0aa..b9eb86cb 100755
> --- a/check
> +++ b/check
> @@ -509,7 +509,7 @@ _expunge_test()
>  	return 0
>  }
>  
> -_init_kmemleak
> +_detect_kmemleak
>  _prepare_test_list
>  
>  if $OPTIONS_HAVE_SECTIONS; then
> @@ -793,8 +793,8 @@ for section in $HOST_OPTIONS_SECTIONS; do
>  			# and log messages that shouldn't be there.
>  			_check_filesystems
>  			_check_dmesg || err=true
> -			_check_kmemleak || err=true
>  		fi
> +		_check_kmemleak || err=true

So we check for kmemleak after each test even when the test already
failed, better to have some comments here to explain why this is
necessary.

>  
>  		# test ends after all checks are done.
>  		$timestamp && _timestamp
> diff --git a/common/rc b/common/rc
> index 19e947df..75771f31 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3514,7 +3514,7 @@ _check_dmesg()
>  # capture the kmemleak report
>  _capture_kmemleak()
>  {
> -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> +	local kern_knob="$DEBUGFS_MNT/kmemleak"

Just wondering why the "{}" are removed in this patch?

>  	local leak_file="$1"
>  
>  	# Tell the kernel to scan for memory leaks.  Apparently the write
> @@ -3535,17 +3535,20 @@ ENDL
>  	echo "clear" > "$kern_knob"
>  }
>  
> -# set up kmemleak
> -_init_kmemleak()
> +# Figure out if the running kernel supports kmemleak; if it does, clear out
> +# anything that leaked before we even started testing.  The leak checker only
> +# needs to be primed like this once per ./check invocation.
> +_detect_kmemleak()
>  {
> -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> +	local kern_knob="$DEBUGFS_MNT/kmemleak"
> +	KMEMLEAK_CHECK_FILE="/tmp/check_kmemleak"

So we're checking the "/tmp/check_kmemleak" file instead of
${RESULT_BASE}/check_kmemleak now, but from the commit log it seems that
it should be ${RESULT_DIR}/check_kmemleak?

>  
>  	# Since kernel v4.19-rc3, the kmemleak knob exists even if kmemleak is
>  	# disabled, but returns EBUSY on write. So instead of relying on
>  	# existance of writable knob file, we use a test file to indicate that
>  	# _check_kmemleak() is enabled only if we actually managed to write to
>  	# the knob file.
> -	rm -f ${RESULT_BASE}/check_kmemleak
> +	rm -f "$KMEMLEAK_CHECK_FILE"
>  
>  	if [ ! -w "$kern_knob" ]; then
>  		return 0
> @@ -3555,17 +3558,26 @@ _init_kmemleak()
>  	# then dump all the leaks recorded so far.
>  	if echo "scan=off" > "$kern_knob" 2>/dev/null; then
>  		_capture_kmemleak /dev/null
> -		touch ${RESULT_BASE}/check_kmemleak
> +		touch "$KMEMLEAK_CHECK_FILE"
>  	fi
>  }
>  
> -# check kmemleak log
> +# Kick the kmemleak checker to scan for leaks.  Background leak scan mode is
> +# not enabled, so we must call the kernel to ask for a scan and deal with the
> +# results appropriately.  This we do after every test completes, whether or not
> +# it was successful.
>  _check_kmemleak()
>  {
> -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> -	local leak_file="${seqres}.kmemleak"
> +	local kern_knob="$DEBUGFS_MNT/kmemleak"
> +	local leak_file="$seqres.kmemleak"
>  
> -	if [ ! -f ${RESULT_BASE}/check_kmemleak ]; then
> +	if [ ! -f "$KMEMLEAK_CHECK_FILE" ]; then
> +		return 0
> +	fi
> +
> +	# Not enabled, so discard any report of leaks found.
> +	if [ "$USE_KMEMLEAK" != "yes" ]; then
> +		_capture_kmemleak /dev/null

New knob requires new documentation in README :)

Thanks,
Eryu

>  		return 0
>  	fi
>  
>
Darrick J. Wong Feb. 6, 2019, 5:06 p.m. UTC | #2
On Sun, Feb 03, 2019 at 05:10:49PM +0800, Eryu Guan wrote:
> On Tue, Jan 29, 2019 at 08:17:25AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor the kmemleak code to work correctly with sections.  This
> 
> Thanks for the fix!
> 
> > requires changing the report location to use RESULT_DIR instead of
> > RESULT_BASE, and clarifying which functions get used when.
> 
> But I didn't see any RESULT_DIR related changes in this patch.

I forgot to update the commit log. :(

> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  check     |    4 ++--
> >  common/rc |   32 ++++++++++++++++++++++----------
> >  2 files changed, 24 insertions(+), 12 deletions(-)
> > 
> > 
> > diff --git a/check b/check
> > index c0eee0aa..b9eb86cb 100755
> > --- a/check
> > +++ b/check
> > @@ -509,7 +509,7 @@ _expunge_test()
> >  	return 0
> >  }
> >  
> > -_init_kmemleak
> > +_detect_kmemleak
> >  _prepare_test_list
> >  
> >  if $OPTIONS_HAVE_SECTIONS; then
> > @@ -793,8 +793,8 @@ for section in $HOST_OPTIONS_SECTIONS; do
> >  			# and log messages that shouldn't be there.
> >  			_check_filesystems
> >  			_check_dmesg || err=true
> > -			_check_kmemleak || err=true
> >  		fi
> > +		_check_kmemleak || err=true
> 
> So we check for kmemleak after each test even when the test already
> failed, better to have some comments here to explain why this is
> necessary.

I'll add this in the next version:

	# Scan for memory leaks after every test so that associating
	# a leak to a particular test will be as accurate as possible.

> >  
> >  		# test ends after all checks are done.
> >  		$timestamp && _timestamp
> > diff --git a/common/rc b/common/rc
> > index 19e947df..75771f31 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -3514,7 +3514,7 @@ _check_dmesg()
> >  # capture the kmemleak report
> >  _capture_kmemleak()
> >  {
> > -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> > +	local kern_knob="$DEBUGFS_MNT/kmemleak"
> 
> Just wondering why the "{}" are removed in this patch?

Making the variable dereferencing consistent with the rest of fstests.

> >  	local leak_file="$1"
> >  
> >  	# Tell the kernel to scan for memory leaks.  Apparently the write
> > @@ -3535,17 +3535,20 @@ ENDL
> >  	echo "clear" > "$kern_knob"
> >  }
> >  
> > -# set up kmemleak
> > -_init_kmemleak()
> > +# Figure out if the running kernel supports kmemleak; if it does, clear out
> > +# anything that leaked before we even started testing.  The leak checker only
> > +# needs to be primed like this once per ./check invocation.
> > +_detect_kmemleak()
> >  {
> > -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> > +	local kern_knob="$DEBUGFS_MNT/kmemleak"
> > +	KMEMLEAK_CHECK_FILE="/tmp/check_kmemleak"
> 
> So we're checking the "/tmp/check_kmemleak" file instead of

Right.

> ${RESULT_BASE}/check_kmemleak now, but from the commit log it seems that
> it should be ${RESULT_DIR}/check_kmemleak?

Er... oops.  I'll update the changelog. :)

> >  
> >  	# Since kernel v4.19-rc3, the kmemleak knob exists even if kmemleak is
> >  	# disabled, but returns EBUSY on write. So instead of relying on
> >  	# existance of writable knob file, we use a test file to indicate that
> >  	# _check_kmemleak() is enabled only if we actually managed to write to
> >  	# the knob file.
> > -	rm -f ${RESULT_BASE}/check_kmemleak
> > +	rm -f "$KMEMLEAK_CHECK_FILE"
> >  
> >  	if [ ! -w "$kern_knob" ]; then
> >  		return 0
> > @@ -3555,17 +3558,26 @@ _init_kmemleak()
> >  	# then dump all the leaks recorded so far.
> >  	if echo "scan=off" > "$kern_knob" 2>/dev/null; then
> >  		_capture_kmemleak /dev/null
> > -		touch ${RESULT_BASE}/check_kmemleak
> > +		touch "$KMEMLEAK_CHECK_FILE"
> >  	fi
> >  }
> >  
> > -# check kmemleak log
> > +# Kick the kmemleak checker to scan for leaks.  Background leak scan mode is
> > +# not enabled, so we must call the kernel to ask for a scan and deal with the
> > +# results appropriately.  This we do after every test completes, whether or not
> > +# it was successful.
> >  _check_kmemleak()
> >  {
> > -	local kern_knob="${DEBUGFS_MNT}/kmemleak"
> > -	local leak_file="${seqres}.kmemleak"
> > +	local kern_knob="$DEBUGFS_MNT/kmemleak"
> > +	local leak_file="$seqres.kmemleak"
> >  
> > -	if [ ! -f ${RESULT_BASE}/check_kmemleak ]; then
> > +	if [ ! -f "$KMEMLEAK_CHECK_FILE" ]; then
> > +		return 0
> > +	fi
> > +
> > +	# Not enabled, so discard any report of leaks found.
> > +	if [ "$USE_KMEMLEAK" != "yes" ]; then
> > +		_capture_kmemleak /dev/null
> 
> New knob requires new documentation in README :)

Will do.  Happy New Year, by the way!

--D

> Thanks,
> Eryu
> 
> >  		return 0
> >  	fi
> >  
> >

Patch
diff mbox series

diff --git a/check b/check
index c0eee0aa..b9eb86cb 100755
--- a/check
+++ b/check
@@ -509,7 +509,7 @@  _expunge_test()
 	return 0
 }
 
-_init_kmemleak
+_detect_kmemleak
 _prepare_test_list
 
 if $OPTIONS_HAVE_SECTIONS; then
@@ -793,8 +793,8 @@  for section in $HOST_OPTIONS_SECTIONS; do
 			# and log messages that shouldn't be there.
 			_check_filesystems
 			_check_dmesg || err=true
-			_check_kmemleak || err=true
 		fi
+		_check_kmemleak || err=true
 
 		# test ends after all checks are done.
 		$timestamp && _timestamp
diff --git a/common/rc b/common/rc
index 19e947df..75771f31 100644
--- a/common/rc
+++ b/common/rc
@@ -3514,7 +3514,7 @@  _check_dmesg()
 # capture the kmemleak report
 _capture_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
 	local leak_file="$1"
 
 	# Tell the kernel to scan for memory leaks.  Apparently the write
@@ -3535,17 +3535,20 @@  ENDL
 	echo "clear" > "$kern_knob"
 }
 
-# set up kmemleak
-_init_kmemleak()
+# Figure out if the running kernel supports kmemleak; if it does, clear out
+# anything that leaked before we even started testing.  The leak checker only
+# needs to be primed like this once per ./check invocation.
+_detect_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
+	KMEMLEAK_CHECK_FILE="/tmp/check_kmemleak"
 
 	# Since kernel v4.19-rc3, the kmemleak knob exists even if kmemleak is
 	# disabled, but returns EBUSY on write. So instead of relying on
 	# existance of writable knob file, we use a test file to indicate that
 	# _check_kmemleak() is enabled only if we actually managed to write to
 	# the knob file.
-	rm -f ${RESULT_BASE}/check_kmemleak
+	rm -f "$KMEMLEAK_CHECK_FILE"
 
 	if [ ! -w "$kern_knob" ]; then
 		return 0
@@ -3555,17 +3558,26 @@  _init_kmemleak()
 	# then dump all the leaks recorded so far.
 	if echo "scan=off" > "$kern_knob" 2>/dev/null; then
 		_capture_kmemleak /dev/null
-		touch ${RESULT_BASE}/check_kmemleak
+		touch "$KMEMLEAK_CHECK_FILE"
 	fi
 }
 
-# check kmemleak log
+# Kick the kmemleak checker to scan for leaks.  Background leak scan mode is
+# not enabled, so we must call the kernel to ask for a scan and deal with the
+# results appropriately.  This we do after every test completes, whether or not
+# it was successful.
 _check_kmemleak()
 {
-	local kern_knob="${DEBUGFS_MNT}/kmemleak"
-	local leak_file="${seqres}.kmemleak"
+	local kern_knob="$DEBUGFS_MNT/kmemleak"
+	local leak_file="$seqres.kmemleak"
 
-	if [ ! -f ${RESULT_BASE}/check_kmemleak ]; then
+	if [ ! -f "$KMEMLEAK_CHECK_FILE" ]; then
+		return 0
+	fi
+
+	# Not enabled, so discard any report of leaks found.
+	if [ "$USE_KMEMLEAK" != "yes" ]; then
+		_capture_kmemleak /dev/null
 		return 0
 	fi