Re: [PATCH 07/28] check-parallel: adjust concurrency according to CPU count

[Dave Chinner <david@fromorbit.com>]

On Wed, May 07, 2025 at 12:15:09PM +0530, Nirjhar Roy (IBM) wrote:
> On Thu, 2025-04-17 at 13:00 +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Concurrency is currently hard coded at 64 worker threads. This is
> > too many for small CPU count machines; the idea is to create a
> > sustained load of roughly one test per CPU as they are mostly single
> > threaded/single process tests. The number "64" was chosen because
> > I've been developing this functionality on a 64p VM.
.....
> > diff --git a/check-parallel b/check-parallel
> > index cb5d6aedf..0649a417f 100755
> > --- a/check-parallel
> > +++ b/check-parallel
> > @@ -10,7 +10,7 @@
> >  # the loop devices.
> >  
> >  basedir=""
> > -runners=64
> > +runners=$(getconf _NPROCESSORS_CONF)
> Minor: Not related to this change. Maybe we should have a _get_nproc()
> just like 
> _get_page_size()
> {
> 	echo $(getconf PAGE_SIZE)
> }
> _get_nproc()
> {
> 	echo $(getconf _NPROCESSORS_CONF)
> }
> and replace all the $(getconf _NPROCESSORS_CONF) with $(_get_nproc)

I have thoughts on that.

I think determining test scaling based on the number of CPUs in the
machine is wrong. If I run in a cgroup that limits tests to 4p on a
64p machine, then fstests will think it is running on a 64p machine
rahter than on 4p. That's ... not ideal.

I think there should be a global variable that defines the
concurrency that tests should use to scale load/processes rather
than use the CPU count. Then check/check-parallel can set the
concurrency as they desire and we no longer have to worry about
tests creating excesively huge loads on high CPU count machines
because they were sized to load a small, low concurrency test
system.

i.e. I intend to separate the concurrency with which check-parallel
runs from the concurrency that individual tests use to scale. For
check-parallel, I am thinking of fixing test concurrency scaling at
something like min(nr_cpus, 8), whilst the check-parallel harness
itself uses nr_cpus to determine how many concurrent runners to
instantiate.

> >  runtimes=()
> >  show_test_list=
> > @@ -30,6 +30,7 @@ usage()
> >  
> >  check options
> >      -D <dir>		Directory to run in
> > +    -t <n>		Number of concurrent tests to  run
> Minor: Maybe we should mention the valid range of <n> i.e, 0 to 1024?
> >      -n			Output test list, do not run tests
> 
> Nit: Maybe there is some spacing issue here? "Number of concurrent ..."
> and "Output test..." don't begin together.

Not that I can see, I think this is just email quoting and tabs
doing funky stuff.

> >      -r			randomize test order
> >      --exact-order	run tests in the exact order specified
> > @@ -81,6 +82,7 @@ while [ $# -gt 0 ]; do
> >  	-\? | -h | --help) usage ;;
> >  
> >  	-D)	basedir=$2; shift ;;
> > +	-t)	runners=$2; shift ;;
> >  	-g)	_tl_setup_group $2 ; shift ;;
> >  	-e)	_tl_setup_exclude_tests $2 ; shift ;;
> >  	-E)	_tl_setup_exclude_file $2 ; shift ;;
> > @@ -111,6 +113,11 @@ if [ ! -d "$basedir" ]; then
> >  	echo "Invalid basedir specification"
> >  	usage
> >  fi
> > +if [[ $runners -le 0 || $runners -gt 1024 ]]; then
> > +	echo "Invalid thread specificaton: $runners"
> Minor: Maybe we should mention the valid range of "runners" in this
> error message (0 to 1024)?

/me shrugs.

It's an arbitrary "check-parallel won't ever scale past this" limit
because the diminishing returns from added concurrency that Amdahl's
Law define is already kicking in at 64 threads. If you are running
check-parallel on a >1024p machine, you already know enough to be
able to look at the source code to determine why the error is being
emitted. :)

-Dave.
-- 
Dave Chinner
david@fromorbit.com