Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] xfstests: add a CGROUP configuration option
@ 2020-02-14 20:34 Josef Bacik
  2020-02-17 16:38 ` Brian Foster
  0 siblings, 1 reply; 4+ messages in thread
From: Josef Bacik @ 2020-02-14 20:34 UTC (permalink / raw)
  To: fstests, linux-btrfs

I want to add some extended statistic gathering for xfstests, but it's
tricky to isolate xfstests from the rest of the host applications.  The
most straightforward way to do this is to run every test inside of it's
own cgroup.  From there we can monitor the activity of tasks in the
specific cgroup using BPF.

The support for this is pretty simple, allow users to specify
CGROUP=/path/to/cgroup.  We will create the path if it doesn't already
exist, and validate we can add things to cgroup.procs.  If we cannot
it'll be disabled, otherwise we will use this when we do _run_seq by
echo'ing the bash pid into cgroup.procs, which will cause any children
to run under that cgroup.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 README |  3 +++
 check  | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/README b/README
index 593c1052..722dc170 100644
--- a/README
+++ b/README
@@ -102,6 +102,9 @@ Preparing system for tests:
              - set USE_KMEMLEAK=yes to scan for memory leaks in the kernel
                after every test, if the kernel supports kmemleak.
              - set KEEP_DMESG=yes to keep dmesg log after test
+             - set CGROUP=/path/to/cgroup to create a cgroup to run tests inside
+               of.  The main check will run outside of the cgroup, only the test
+               itself and any child processes will run under the cgroup.
 
         - or add a case to the switch in common/config assigning
           these variables based on the hostname of your test
diff --git a/check b/check
index 2e148e57..07a0e251 100755
--- a/check
+++ b/check
@@ -509,11 +509,23 @@ _expunge_test()
 OOM_SCORE_ADJ="/proc/self/oom_score_adj"
 test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
 
+# Initialize the cgroup path if it doesn't already exist
+if [ ! -z "$CGROUP" ]; then
+	mkdir -p ${CGROUP}
+
+	# If we can't write to cgroup.procs then unset cgroup
+	test -w ${CGROUP}/cgroup.procs || unset CGROUP
+fi
+
 # ...and make the tests themselves somewhat more attractive to it, so that if
 # the system runs out of memory it'll be the test that gets killed and not the
 # test framework.
 _run_seq() {
-	bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec ./$seq"
+	_extra="test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ};"
+	if [ ! -z "$CGROUP" ]; then
+		_extra+="echo $$ > ${CGROUP}/cgroup.procs;"
+	fi
+	bash -c "${_extra} exec ./$seq"
 }
 
 _detect_kmemleak
@@ -615,6 +627,9 @@ for section in $HOST_OPTIONS_SECTIONS; do
 	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
 	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
 	fi
+	if [ ! -z "$CGROUP" ]; then
+	  echo "CGROUP        -- ${CGROUP}"
+	fi
 	echo
 	needwrap=true
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xfstests: add a CGROUP configuration option
  2020-02-14 20:34 [PATCH] xfstests: add a CGROUP configuration option Josef Bacik
@ 2020-02-17 16:38 ` Brian Foster
  2020-02-18 14:17   ` Josef Bacik
  0 siblings, 1 reply; 4+ messages in thread
From: Brian Foster @ 2020-02-17 16:38 UTC (permalink / raw)
  To: Josef Bacik; +Cc: fstests, linux-btrfs

On Fri, Feb 14, 2020 at 03:34:31PM -0500, Josef Bacik wrote:
> I want to add some extended statistic gathering for xfstests, but it's
> tricky to isolate xfstests from the rest of the host applications.  The
> most straightforward way to do this is to run every test inside of it's
> own cgroup.  From there we can monitor the activity of tasks in the
> specific cgroup using BPF.
> 

I'm curious what kind of info you're looking for from tests..

> The support for this is pretty simple, allow users to specify
> CGROUP=/path/to/cgroup.  We will create the path if it doesn't already
> exist, and validate we can add things to cgroup.procs.  If we cannot
> it'll be disabled, otherwise we will use this when we do _run_seq by
> echo'ing the bash pid into cgroup.procs, which will cause any children
> to run under that cgroup.
> 

Seems reasonable, but is there any opportunity to combine this with what
we have in common/cgroup2? It's not clear to me if this cares about
cgroup v1 or v2, but perhaps the cgroup2 checks could be built on top of
a generic CGROUP var? I'm also wondering if we'd want to change runtime
behavior purely based on the existence of the path as opposed to some
kind of separate knob (in the event some future test requires the path
for some reason without wanting to enable this mechanism).

Brian

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  README |  3 +++
>  check  | 17 ++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/README b/README
> index 593c1052..722dc170 100644
> --- a/README
> +++ b/README
> @@ -102,6 +102,9 @@ Preparing system for tests:
>               - set USE_KMEMLEAK=yes to scan for memory leaks in the kernel
>                 after every test, if the kernel supports kmemleak.
>               - set KEEP_DMESG=yes to keep dmesg log after test
> +             - set CGROUP=/path/to/cgroup to create a cgroup to run tests inside
> +               of.  The main check will run outside of the cgroup, only the test
> +               itself and any child processes will run under the cgroup.
>  
>          - or add a case to the switch in common/config assigning
>            these variables based on the hostname of your test
> diff --git a/check b/check
> index 2e148e57..07a0e251 100755
> --- a/check
> +++ b/check
> @@ -509,11 +509,23 @@ _expunge_test()
>  OOM_SCORE_ADJ="/proc/self/oom_score_adj"
>  test -w ${OOM_SCORE_ADJ} && echo -1000 > ${OOM_SCORE_ADJ}
>  
> +# Initialize the cgroup path if it doesn't already exist
> +if [ ! -z "$CGROUP" ]; then
> +	mkdir -p ${CGROUP}
> +
> +	# If we can't write to cgroup.procs then unset cgroup
> +	test -w ${CGROUP}/cgroup.procs || unset CGROUP
> +fi
> +
>  # ...and make the tests themselves somewhat more attractive to it, so that if
>  # the system runs out of memory it'll be the test that gets killed and not the
>  # test framework.
>  _run_seq() {
> -	bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec ./$seq"
> +	_extra="test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ};"
> +	if [ ! -z "$CGROUP" ]; then
> +		_extra+="echo $$ > ${CGROUP}/cgroup.procs;"
> +	fi
> +	bash -c "${_extra} exec ./$seq"
>  }
>  
>  _detect_kmemleak
> @@ -615,6 +627,9 @@ for section in $HOST_OPTIONS_SECTIONS; do
>  	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
>  	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
>  	fi
> +	if [ ! -z "$CGROUP" ]; then
> +	  echo "CGROUP        -- ${CGROUP}"
> +	fi
>  	echo
>  	needwrap=true
>  
> -- 
> 2.24.1
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xfstests: add a CGROUP configuration option
  2020-02-17 16:38 ` Brian Foster
@ 2020-02-18 14:17   ` Josef Bacik
  2020-02-18 15:40     ` Brian Foster
  0 siblings, 1 reply; 4+ messages in thread
From: Josef Bacik @ 2020-02-18 14:17 UTC (permalink / raw)
  To: Brian Foster; +Cc: fstests, linux-btrfs

On 2/17/20 11:38 AM, Brian Foster wrote:
> On Fri, Feb 14, 2020 at 03:34:31PM -0500, Josef Bacik wrote:
>> I want to add some extended statistic gathering for xfstests, but it's
>> tricky to isolate xfstests from the rest of the host applications.  The
>> most straightforward way to do this is to run every test inside of it's
>> own cgroup.  From there we can monitor the activity of tasks in the
>> specific cgroup using BPF.
>>
> 
> I'm curious what kind of info you're looking for from tests..
> 

Latencies.  We have all of these tests doing all sorts of interesting things, I 
want to track operation latencies with code we're actually testing so I can see 
if I've introduced a performance regression somewhere.  Since Facebook's whole 
fleet is on btrfs I want to make sure I'm only getting information from things 
being run by xfstests so I can easily go back and hunt down regressions that get 
introduced.  With BPF I can filter on cgroup membership, so I know I'm only 
recording stats I care about.

>> The support for this is pretty simple, allow users to specify
>> CGROUP=/path/to/cgroup.  We will create the path if it doesn't already
>> exist, and validate we can add things to cgroup.procs.  If we cannot
>> it'll be disabled, otherwise we will use this when we do _run_seq by
>> echo'ing the bash pid into cgroup.procs, which will cause any children
>> to run under that cgroup.
>>
> 
> Seems reasonable, but is there any opportunity to combine this with what
> we have in common/cgroup2? It's not clear to me if this cares about
> cgroup v1 or v2, but perhaps the cgroup2 checks could be built on top of
> a generic CGROUP var? I'm also wondering if we'd want to change runtime
> behavior purely based on the existence of the path as opposed to some
> kind of separate knob (in the event some future test requires the path
> for some reason without wanting to enable this mechanism).
> 

Oh I probably should have looked around, yeah we can definitely use this.  My 
initial thought is to just make CGROUP2_PATH exported always, we create 
/path/to/cgroup2/xfstests and point CGROUP2_PATH at that, and then any tests 
that use the cgroup2 path for their test will automatically be populated under 
that global xfstests directory, so I can still capture them with my scripts. 
Does that sound reasonable?  Thanks,

Josef

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] xfstests: add a CGROUP configuration option
  2020-02-18 14:17   ` Josef Bacik
@ 2020-02-18 15:40     ` Brian Foster
  0 siblings, 0 replies; 4+ messages in thread
From: Brian Foster @ 2020-02-18 15:40 UTC (permalink / raw)
  To: Josef Bacik; +Cc: fstests, linux-btrfs

On Tue, Feb 18, 2020 at 09:17:10AM -0500, Josef Bacik wrote:
> On 2/17/20 11:38 AM, Brian Foster wrote:
> > On Fri, Feb 14, 2020 at 03:34:31PM -0500, Josef Bacik wrote:
> > > I want to add some extended statistic gathering for xfstests, but it's
> > > tricky to isolate xfstests from the rest of the host applications.  The
> > > most straightforward way to do this is to run every test inside of it's
> > > own cgroup.  From there we can monitor the activity of tasks in the
> > > specific cgroup using BPF.
> > > 
> > 
> > I'm curious what kind of info you're looking for from tests..
> > 
> 
> Latencies.  We have all of these tests doing all sorts of interesting
> things, I want to track operation latencies with code we're actually testing
> so I can see if I've introduced a performance regression somewhere.  Since
> Facebook's whole fleet is on btrfs I want to make sure I'm only getting
> information from things being run by xfstests so I can easily go back and
> hunt down regressions that get introduced.  With BPF I can filter on cgroup
> membership, so I know I'm only recording stats I care about.
> 

Interesting, might be useful to document the use case in the commit log.

> > > The support for this is pretty simple, allow users to specify
> > > CGROUP=/path/to/cgroup.  We will create the path if it doesn't already
> > > exist, and validate we can add things to cgroup.procs.  If we cannot
> > > it'll be disabled, otherwise we will use this when we do _run_seq by
> > > echo'ing the bash pid into cgroup.procs, which will cause any children
> > > to run under that cgroup.
> > > 
> > 
> > Seems reasonable, but is there any opportunity to combine this with what
> > we have in common/cgroup2? It's not clear to me if this cares about
> > cgroup v1 or v2, but perhaps the cgroup2 checks could be built on top of
> > a generic CGROUP var? I'm also wondering if we'd want to change runtime
> > behavior purely based on the existence of the path as opposed to some
> > kind of separate knob (in the event some future test requires the path
> > for some reason without wanting to enable this mechanism).
> > 
> 
> Oh I probably should have looked around, yeah we can definitely use this.
> My initial thought is to just make CGROUP2_PATH exported always, we create
> /path/to/cgroup2/xfstests and point CGROUP2_PATH at that, and then any tests
> that use the cgroup2 path for their test will automatically be populated
> under that global xfstests directory, so I can still capture them with my
> scripts. Does that sound reasonable?  Thanks,
> 

Not sure I follow.. are you saying that we'd change CGROUP2_PATH from
simply pointing at the local cgroup2 root on the local box to some magic
field that directs creation of a cgroup for the particular test? Or did
you mean to use a different variable name in the second case? Maybe it's
just easier to see a patch...

Brian

> Josef
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 20:34 [PATCH] xfstests: add a CGROUP configuration option Josef Bacik
2020-02-17 16:38 ` Brian Foster
2020-02-18 14:17   ` Josef Bacik
2020-02-18 15:40     ` Brian Foster

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git