Use compact process binding for GROMACS

[casparvl]

Without process binding, mpirun was binding processes to NUMA domains. I'm not sure why, but occasionally I saw some cores on Snellius being empty, while others seemed to run 2 processes. These runs would run about 10x slower than normal. This must be some weird behaviour from the OS thread scheduler. With process binding, performance is more consistent: while I still see some variation, I don't see the extremely slow runs anymore. Also, it seems performance also went up a bit (few %) on average, though it is a bit hard to be sure with the natural variation between runs.

Depends on:

• Fix compact process binding for OpenMPI mpirun #137

[casparvl]

Fixes #138

[casparvl]

Hm, not sure why https://github.com/EESSI/test-suite/actions/runs/8942074753/job/24563848640?pr=139 is happening:

def set_compact_process_binding(test: rfm.RegressionTest):
...
    check_proc_attribute_defined(test, 'num_cpus_per_core')
    num_cpus_per_core = test.current_partition.processor.num_cpus_per_core
    physical_cpus_per_task = int(test.num_cpus_per_task / num_cpus_per_core)
...

The check_proc_attribute_defined should have printed a clear error that this property wasn't defined in the ReFrame config file. Also, I'm not sure why we wouldn't have run into this before, but I guess the CI test wasn't there before...

The solution will be to make sure that num_cpus_per_core is defined in the CI config file... I'll check how we can achieve that.

[smoors]

The check_proc_attribute_defined should have printed a clear error that this property wasn't defined in the ReFrame config file.

there is a bug in check_proc_attribute_defined: the last line (the raise) has 1 indent too many

[smoors]

for slurm versions >= 22.05 < 23.11 when using srun, there is another issue: --cpus-per-task is not inherited from the job environment into the srun tasks, preventing "bind each process to subsequent domains of test.num_cpus_per_task cores".

would you mind adding the following to the hook set_compact_process_binding to fix this?

    if test.current_partition.launcher_type().registered_name == 'srun':
        test.env_vars['SRUN_CPUS_PER_TASK'] = test.num_cpus_per_task

[smoors]

more testing fun with srun.

by default, srun does a block distribution over nodes, but a cyclic distribution over sockets, which is different from mpirun with test.env_vars['OMPI_MCA_rmaps_base_mapping_policy'] = 'slot:PE=%s' % physical_cpus_per_task, which does block distribution over sockets.
to get the same behavior as mpirun we can use environment variable $SLURM_DISTRIBUTION. i propose to only set all srun specific environments when srun is actually used as a launcher:

   if test.current_partition.launcher_type().registered_name == 'srun':
       test.env_vars['SRUN_CPUS_PER_TASK'] = test.num_cpus_per_task
       test.env_vars['SLURM_DISTRIBUTION'] = 'block:block'
       test.env_vars['SLURM_CPU_BIND'] = 'verbose'

[casparvl]

srun does a block distribution over nodes, but a cyclic distribution over sockets

Good catch, I didn't realize! I think it makes sense what you are proposing and to do this only if srun is used as launcher.

A few things I am wondering regarding

test.env_vars['SRUN_CPUS_PER_TASK'] = test.num_cpus_per_task

1. I only see a SLURM_CPUS_PER_TASK in the srun and sbatch manuals https://slurm.schedmd.com/srun.html , https://slurm.schedmd.com/sbatch.html . Did you mean that?
2. As I understand it, SLURM_CPUS_PER_TASK is an input environment variable for srun, i.e. it does the same as when you would invoke srun -c $SLURM_CPUS_PER_TASK. This means it determines the size of the allocation for the job step that srun creates, doesn't it? In that case, I'm not sure if the process-binding hook is the right place to set it, it seems more general to me. Shouldn't it also be set for tests that don't care about process binding?
3. Should we fix things that are broken in SLURM in our test suite? SLURM_CPUS_PER_TASK is a documented output environment variable for sbatch. If it's not set, that's a bug in SLURM. It means that all jobs that rely on srun picking up the correct number of cpus per task from the parent allocation suddenly fail, or behave differently. If that's the case, I'm ok with the tests failing too, no?

[smoors]

I only see a SLURM_CPUS_PER_TASK in the srun and sbatch manuals https://slurm.schedmd.com/srun.html , https://slurm.schedmd.com/sbatch.html . Did you mean that?

i mean SRUN_CPUS_PER_TASK, see for example the docs for v22.05: https://slurm.schedmd.com/archive/slurm-22.05.0/srun.html#OPT_cpus-per-task
they have now reversed the behavior back to the old behavior and thus SRUN_CPUS_PER_TASK is no longer needed.

As I understand it, SLURM_CPUS_PER_TASK is an input environment variable for srun, i.e. it does the same as when you would invoke srun -c $SLURM_CPUS_PER_TASK. This means it determines the size of the allocation for the job step that srun creates, doesn't it? In that case, I'm not sure if the process-binding hook is the right place to set it, it seems more general to me. Shouldn't it also be set for tests that don't care about process binding?

it doesn't determine the allocation of the job step, only the binding of the tasks. i agree though it's better to always set it so the behavior is always the same regardless of the slurm version being used.

Should we fix things that are broken in SLURM in our test suite? SLURM_CPUS_PER_TASK is a documented output environment variable for sbatch. If it's not set, that's a bug in SLURM. It means that all jobs that rely on srun picking up the correct number of cpus per task from the parent allocation suddenly fail, or behave differently. If that's the case, I'm ok with the tests failing too, no?

it's not a bug, it is intended behavior for the range of slurm versions i mentioned. they have now reversed this behavior to before 22.05.

[smoors]

i'll make a separate PR for the SRUN_CPUS_PER_TASK issue

[smoors]

add log for SLURM_DISTRIBUTION here, and move log for SLURM_CPU_BIND in this if block?

[casparvl]

Done. Also added a log for I_MPI_PIN_CELL, since that was missing too. I also nested the mpirun variables in a seperate if. Additionally, I added a warning message if a launcher is used that we do not support in this function. It's not a blocker, so I won't abort the test, but the user should be aware that performance might be sub-par if the default binding strategy isn't great.

[casparvl]

i mean SRUN_CPUS_PER_TASK, see for example the docs for v22.05: https://slurm.schedmd.com/archive/slurm-22.05.0/srun.html#OPT_cpus-per-task
they have now reversed the behavior back to the old behavior and thus SRUN_CPUS_PER_TASK is no longer needed.

...

it's not a bug, it is intended behavior for the range of slurm versions i mentioned. they have now reversed this behavior to before 22.05.

Oh, I didn't realize that. Weird change, and I'm happy they changed it back :D

it doesn't determine the allocation of the job step, only the binding of the tasks. i agree though it's better to always set it so the behavior is always the same regardless of the slurm version being used.

Interesting, I never realized. I thought that srun -c 2 would create a cgroup with two CPU cores per task, just like it does for srun --mem=<something>. But you're right, it doesn't, it indeed only sets the affinity of the process:

[casparl@int5 ~]$ salloc -n 2 --ntasks-per-node 2 -c 24 -t 10:00 -p genoa
...
[casparl@tcn928 ~]$ srun -n 2 -c 2 --mem=2G cat /sys/fs/cgroup/memory/slurm/uid_45397/job_6176002/step_0/memory.limit_in_bytes
2147483648
2147483648
[casparl@tcn928 ~]$ srun -n 2 -c 2 --mem=2G cat /sys/fs/cgroup/cpuset/slurm/uid_45397/job_6176002/step_1/cpuset.cpus
64-65,80-81
64-65,80-81
[casparl@tcn739 ~]$ srun -c 2 numactl --show
...
physcpubind: 80 81
...
physcpubind: 64 65

But agreed, it's probably still good to set this in any case at a more general level, even if the result is a form of binding. The issue if we don't set it is that we don't control what the default affinity is - and if that's a single core, a test that we think doesn't use process binding will still be bound (to a single core). That's awkward. If you can create a PR for that: great!

[smoors]

tested for openmpi with mpirun and srun, seems to work fine