[LTP] [PATCH v3 0/4] syscalls/msgstress: fixes for small systems

[LTP] [PATCH v3 0/4] syscalls/msgstress: fixes for small systems

[Krzysztof Kozlowski]

Hi,

Changes since v2:
1. Resend due to messed up previous submission.

Changes since v1:
1. Move the code reading cgroups session limit to lib/tst_pid.c to
   existing tst_get_free_pids_().
2. Allow reading session limits from cgroups v2.
3. Add patch 1/4 - typo fix.
4. Add patch 4/4 with the buffer/reserve of pids.

Best regards,
Krzysztof

Krzysztof Kozlowski (4):
  include/tst_pid.h: fix language typo (subtraction)
  syscalls/msgstress04: fix fork failure on small memory systems
  syscalls/msgstress03: fix fork failure on small memory systems
  syscalls/msgstress: tune limit of processes for small machines

 include/tst_pid.h                             |  6 +-
 lib/tst_pid.c                                 | 81 ++++++++++++++++++-
 .../syscalls/ipc/msgstress/msgstress03.c      | 15 +++-
 3 files changed, 98 insertions(+), 4 deletions(-)

-- 
2.27.0

[LTP] [PATCH v3 1/4] include/tst_pid.h: fix language typo (subtraction)

[Krzysztof Kozlowski]

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 include/tst_pid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/tst_pid.h b/include/tst_pid.h
index 9ba1abb27b7c..6c42f73a57e7 100644
--- a/include/tst_pid.h
+++ b/include/tst_pid.h
@@ -13,7 +13,7 @@
 pid_t tst_get_unused_pid_(void (*cleanup_fn)(void));
 
 /*
- * Returns number of free pids by substarction of the number of pids
+ * Returns number of free pids by subtraction of the number of pids
  * currently used ('ps -eT') from max_pids
  */
 int tst_get_free_pids_(void (*cleanup_fn)(void));
-- 
2.27.0

[LTP] [PATCH v3 1/4] include/tst_pid.h: fix language typo (subtraction)

[Petr Vorel]

Hi Krzysztof,

>  /*
> - * Returns number of free pids by substarction of the number of pids
> + * Returns number of free pids by subtraction of the number of pids
>   * currently used ('ps -eT') from max_pids
Because you change the description of this function in the next commit,
I'd just fix this simple typo in 2nd commit. But of course it's not wrong
(prevents keeping the fix even if we'd removed second commit).

Kind regards,
Petr
>   */
>  int tst_get_free_pids_(void (*cleanup_fn)(void));

[LTP] [PATCH v3 1/4] include/tst_pid.h: fix language typo (subtraction)

[Krzysztof Kozlowski]

On 28/06/2021 16:49, Petr Vorel wrote:
> Hi Krzysztof,
> 
>>  /*
>> - * Returns number of free pids by substarction of the number of pids
>> + * Returns number of free pids by subtraction of the number of pids
>>   * currently used ('ps -eT') from max_pids
> Because you change the description of this function in the next commit,
> I'd just fix this simple typo in 2nd commit. But of course it's not wrong
> (prevents keeping the fix even if we'd removed second commit).

The commits should do only one thing (fix one bug, bring one feature if
possible) and the next commit does not touch that line. There is no
re-doing of the same change in next commit.

Best regards,
Krzysztof

[LTP] [PATCH v3 1/4] include/tst_pid.h: fix language typo (subtraction)

[Petr Vorel]

> On 28/06/2021 16:49, Petr Vorel wrote:
> > Hi Krzysztof,

> >>  /*
> >> - * Returns number of free pids by substarction of the number of pids
> >> + * Returns number of free pids by subtraction of the number of pids
> >>   * currently used ('ps -eT') from max_pids
> > Because you change the description of this function in the next commit,
> > I'd just fix this simple typo in 2nd commit. But of course it's not wrong
> > (prevents keeping the fix even if we'd removed second commit).

> The commits should do only one thing (fix one bug, bring one feature if
> possible) and the next commit does not touch that line. There is no
> re-doing of the same change in next commit.
As I wrote in this case I wouldn't bother (with test rewrite to new API there
are much more changes in single commit), but sure you're right this is the best
practice which we follow.

Kind regards,
Petr

> Best regards,
> Krzysztof

[LTP] [PATCH v3 2/4] syscalls/msgstress04: fix fork failure on small memory systems

[Krzysztof Kozlowski]

Running syscalls/msgstress04 on a system with less than ~4 GB of RAM fails:

    Fork failure in the first child of child group 4396
    Fork failure in the second child of child group 4413
    msgstress04    1  TFAIL  :  msgstress04.c:222: Child exit status = 1

The reason is cgroups pid limit set by systemd user.slice.  The limit is
set for login session, also for root user.  For example on 2 GB RAM
machine it is set as:
    /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207

Read the maximum number of pids and adjust the test limit.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 include/tst_pid.h |  4 ++-
 lib/tst_pid.c     | 79 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/include/tst_pid.h b/include/tst_pid.h
index 6c42f73a57e7..8d999a655f1a 100644
--- a/include/tst_pid.h
+++ b/include/tst_pid.h
@@ -14,7 +14,9 @@ pid_t tst_get_unused_pid_(void (*cleanup_fn)(void));
 
 /*
  * Returns number of free pids by subtraction of the number of pids
- * currently used ('ps -eT') from max_pids
+ * currently used ('ps -eT') from maximum number of processes.
+ * The limit of processes come from kernel pid_max and cgroup session limits
+ * (e.g. configured by systemd user.slice).
  */
 int tst_get_free_pids_(void (*cleanup_fn)(void));
 
diff --git a/lib/tst_pid.c b/lib/tst_pid.c
index 9568cc9e91d2..c1ea3fe90e83 100644
--- a/lib/tst_pid.c
+++ b/lib/tst_pid.c
@@ -18,14 +18,20 @@
  *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <sys/types.h>
+#include <unistd.h>
 #include "test.h"
 #include "tst_pid.h"
 #include "old_safe_file_ops.h"
 
 #define PID_MAX_PATH "/proc/sys/kernel/pid_max"
+#define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max"
+#define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max"
 
 pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
 {
@@ -36,10 +42,77 @@ pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
 	return pid;
 }
 
+/*
+ * Get the effective session UID - either one invoking current test via sudo
+ * or the real UID.
+ */
+static int get_session_uid(void)
+{
+	const char *sudo_uid;
+
+	sudo_uid = getenv("SUDO_UID");
+	if (sudo_uid) {
+		int real_uid;
+
+		TEST(sscanf(sudo_uid, "%u", &real_uid));
+		if (TEST_RETURN != 1) {
+#ifdef DEBUG
+			tst_resm(TINFO, "No SUDO_UID from env");
+#endif
+		} else {
+			return real_uid;
+		}
+	}
+
+	return getuid();
+}
+
+static int read_session_pids_limit(const char *path_fmt, int uid,
+				   void (*cleanup_fn) (void))
+{
+	int max_pids, ret;
+	char path[PATH_MAX];
+
+	ret = snprintf(path, sizeof(path), path_fmt, uid);
+	if (ret < 0 || (size_t)ret >= sizeof(path))
+		return -1;
+
+	if (access(path, R_OK) != 0) {
+		tst_resm(TINFO, "Cannot read session user limits from '%s'", path);
+		return -1;
+	}
+
+	SAFE_FILE_SCANF(cleanup_fn, path, "%d", &max_pids);
+	tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, path);
+
+	return max_pids;
+}
+
+static int get_session_pids_limit(void (*cleanup_fn) (void))
+{
+	int max_pids, uid;
+
+	uid = get_session_uid();
+	if (TEST_RETURN != 1) {
+		tst_resm(TINFO, "Cannot get UID");
+		return -1;
+	}
+
+	max_pids = read_session_pids_limit(CGROUPS_V2_SLICE_FMT, uid, cleanup_fn);
+	if (max_pids < 0)
+		max_pids = read_session_pids_limit(CGROUPS_V1_SLICE_FMT, uid,
+						   cleanup_fn);
+
+	if (max_pids < 0)
+		return -1;
+
+	return max_pids;
+}
+
 int tst_get_free_pids_(void (*cleanup_fn) (void))
 {
 	FILE *f;
-	int rc, used_pids, max_pids;
+	int rc, used_pids, max_pids, max_session_pids;
 
 	f = popen("ps -eT | wc -l", "r");
 	if (!f) {
@@ -57,6 +130,10 @@ int tst_get_free_pids_(void (*cleanup_fn) (void))
 
 	SAFE_FILE_SCANF(cleanup_fn, PID_MAX_PATH, "%d", &max_pids);
 
+	max_session_pids = get_session_pids_limit(cleanup_fn);
+	if ((max_session_pids > 0) && (max_session_pids < max_pids))
+		max_pids = max_session_pids;
+
 	/* max_pids contains the maximum PID + 1,
 	 * used_pids contains used PIDs + 1,
 	 * so this additional '1' is eliminated by the substraction */
-- 
2.27.0

[LTP] [PATCH v3 2/4] syscalls/msgstress04: fix fork failure on small memory systems

[Petr Vorel]

Hi Krzysztof,

nit: Instead of git commit subject "syscalls/msgstress04: fix fork failure
on small memory systems". I'd use "tst_pid.c: fix fork ...".

> Running syscalls/msgstress04 on a system with less than ~4 GB of RAM fails:

>     Fork failure in the first child of child group 4396
>     Fork failure in the second child of child group 4413
>     msgstress04    1  TFAIL  :  msgstress04.c:222: Child exit status = 1

> The reason is cgroups pid limit set by systemd user.slice.  The limit is
> set for login session, also for root user.  For example on 2 GB RAM
> machine it is set as:
>     /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207

> Read the maximum number of pids and adjust the test limit.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  include/tst_pid.h |  4 ++-
>  lib/tst_pid.c     | 79 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 81 insertions(+), 2 deletions(-)

> diff --git a/include/tst_pid.h b/include/tst_pid.h
> index 6c42f73a57e7..8d999a655f1a 100644
> --- a/include/tst_pid.h
> +++ b/include/tst_pid.h
> @@ -14,7 +14,9 @@ pid_t tst_get_unused_pid_(void (*cleanup_fn)(void));

>  /*
>   * Returns number of free pids by subtraction of the number of pids
> - * currently used ('ps -eT') from max_pids
> + * currently used ('ps -eT') from maximum number of processes.
> + * The limit of processes come from kernel pid_max and cgroup session limits
> + * (e.g. configured by systemd user.slice).
>   */
>  int tst_get_free_pids_(void (*cleanup_fn)(void));

> diff --git a/lib/tst_pid.c b/lib/tst_pid.c
> index 9568cc9e91d2..c1ea3fe90e83 100644
> --- a/lib/tst_pid.c
> +++ b/lib/tst_pid.c
> @@ -18,14 +18,20 @@
>   *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */

> +#include <errno.h>
>  #include <fcntl.h>
>  #include <limits.h>
> +#include <stdio.h>
> +#include <stdlib.h>
>  #include <sys/types.h>
> +#include <unistd.h>
>  #include "test.h"
>  #include "tst_pid.h"
>  #include "old_safe_file_ops.h"

>  #define PID_MAX_PATH "/proc/sys/kernel/pid_max"
> +#define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max"
> +#define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max"

>  pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
>  {
> @@ -36,10 +42,77 @@ pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
>  	return pid;
>  }

> +/*
> + * Get the effective session UID - either one invoking current test via sudo
> + * or the real UID.
> + */
> +static int get_session_uid(void)
> +{
> +	const char *sudo_uid;
> +
> +	sudo_uid = getenv("SUDO_UID");
> +	if (sudo_uid) {
> +		int real_uid;
> +
> +		TEST(sscanf(sudo_uid, "%u", &real_uid));
FYI We recently decided to not use TEST() macro in library itself.
See Richard's effort [1]. We should document it in LTP Library API Writing Guidelines [2]

> +		if (TEST_RETURN != 1) {
> +#ifdef DEBUG
FYI we don't support DEBUG. Either the information is always important or not.
In this case I'd probably avoid printing it.

> +			tst_resm(TINFO, "No SUDO_UID from env");
> +#endif
> +		} else {
> +			return real_uid;
> +		}
> +	}
> +
> +	return getuid();
> +}
> +
> +static int read_session_pids_limit(const char *path_fmt, int uid,
> +				   void (*cleanup_fn) (void))
> +{
> +	int max_pids, ret;
> +	char path[PATH_MAX];
> +
> +	ret = snprintf(path, sizeof(path), path_fmt, uid);
> +	if (ret < 0 || (size_t)ret >= sizeof(path))
> +		return -1;
> +
> +	if (access(path, R_OK) != 0) {
> +		tst_resm(TINFO, "Cannot read session user limits from '%s'", path);
> +		return -1;
> +	}
> +
> +	SAFE_FILE_SCANF(cleanup_fn, path, "%d", &max_pids);
> +	tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, path);
> +
> +	return max_pids;
> +}
> +
> +static int get_session_pids_limit(void (*cleanup_fn) (void))
> +{
> +	int max_pids, uid;
> +
> +	uid = get_session_uid();
> +	if (TEST_RETURN != 1) {
and here as well 
> +		tst_resm(TINFO, "Cannot get UID");
> +		return -1;
> +	}

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20210621113804.26179-2-rpalethorpe@suse.com/
[2] https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines

[LTP] [PATCH v3 2/4] syscalls/msgstress04: fix fork failure on small memory systems

[Krzysztof Kozlowski]

On 28/06/2021 16:47, Petr Vorel wrote:
> Hi Krzysztof,
> 
> nit: Instead of git commit subject "syscalls/msgstress04: fix fork failure
> on small memory systems". I'd use "tst_pid.c: fix fork ...".

Sure.

>> +/*
>> + * Get the effective session UID - either one invoking current test via sudo
>> + * or the real UID.
>> + */
>> +static int get_session_uid(void)
>> +{
>> +	const char *sudo_uid;
>> +
>> +	sudo_uid = getenv("SUDO_UID");
>> +	if (sudo_uid) {
>> +		int real_uid;
>> +
>> +		TEST(sscanf(sudo_uid, "%u", &real_uid));
> FYI We recently decided to not use TEST() macro in library itself.
> See Richard's effort [1]. We should document it in LTP Library API Writing Guidelines [2]
> 

I'll remove it.

>> +		if (TEST_RETURN != 1) {
>> +#ifdef DEBUG
> FYI we don't support DEBUG. Either the information is always important or not.
> In this case I'd probably avoid printing it.

Sure.

> 
>> +			tst_resm(TINFO, "No SUDO_UID from env");
>> +#endif
>> +		} else {
>> +			return real_uid;
>> +		}
>> +	}
>> +
>> +	return getuid();
>> +}
>> +
>> +static int read_session_pids_limit(const char *path_fmt, int uid,
>> +				   void (*cleanup_fn) (void))
>> +{
>> +	int max_pids, ret;
>> +	char path[PATH_MAX];
>> +
>> +	ret = snprintf(path, sizeof(path), path_fmt, uid);
>> +	if (ret < 0 || (size_t)ret >= sizeof(path))
>> +		return -1;
>> +
>> +	if (access(path, R_OK) != 0) {
>> +		tst_resm(TINFO, "Cannot read session user limits from '%s'", path);
>> +		return -1;
>> +	}
>> +
>> +	SAFE_FILE_SCANF(cleanup_fn, path, "%d", &max_pids);
>> +	tst_resm(TINFO, "Found limit of processes %d (from %s)", max_pids, path);
>> +
>> +	return max_pids;
>> +}
>> +
>> +static int get_session_pids_limit(void (*cleanup_fn) (void))
>> +{
>> +	int max_pids, uid;
>> +
>> +	uid = get_session_uid();
>> +	if (TEST_RETURN != 1) {
> and here as well 

OK, thanks for review!


Best regards,
Krzysztof

[LTP] [PATCH v3 3/4] syscalls/msgstress03: fix fork failure on small memory systems

[Krzysztof Kozlowski]

Running syscalls/msgstress03 on a system with less than ~4 GB of RAM fails:

    msgstress03    1  TFAIL  :  msgstress03.c:155: 	Fork failed (may be OK if under stress)

In dmesg:

    LTP: starting msgstress03
    cgroup: fork rejected by pids controller in /user.slice/user-1000.slice/session-1.scope

The reason is cgroups pid limit set by systemd user.slice.  The limit is
set for login session, also for root user.  For example on 2 GB RAM
machine it is set as:
    /sys/fs/cgroup/pids/user.slice/user-0.slice/pids.max:5207

Read the maximum number of pids and adjust the test limit.  For 2 GB RAM
machine with systemd this will result in:

    msgstress03    0  TINFO  :  Found limit of processes 5056 (from /sys/fs/cgroup/pids/user.slice/user-1000.slice/pids.max)
    msgstress03    0  TINFO  :  Requested number of processes higher than user session limit (10000 > 4556), setting to 4556

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 .../kernel/syscalls/ipc/msgstress/msgstress03.c   | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
index 294b401b1b38..18e50e35ee07 100644
--- a/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
+++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress03.c
@@ -78,7 +78,7 @@ static void usage(void)
 
 int main(int argc, char **argv)
 {
-	int i, j, ok, pid;
+	int i, j, ok, pid, free_pids;
 	int count, status;
 	struct sigaction act;
 
@@ -109,6 +109,19 @@ int main(int argc, char **argv)
 		}
 	}
 
+	free_pids = tst_get_free_pids(cleanup);
+	if (free_pids < 0) {
+		tst_brkm(TBROK, cleanup, "Can't obtain free_pid count");
+	} else if (!free_pids) {
+		tst_brkm(TBROK, cleanup, "No free pids");
+	}
+	if (nprocs >= free_pids) {
+		tst_resm(TINFO,
+			 "Requested number of processes higher than limit (%d > %d), "
+			 "setting to %d", nprocs, free_pids, free_pids);
+		nprocs = free_pids;
+	}
+
 	srand(getpid());
 	tid = -1;
 
-- 
2.27.0

[LTP] [PATCH v3 4/4] syscalls/msgstress: tune limit of processes for small machines

[Krzysztof Kozlowski]

Forking the exactly amount of processes as the limit (either from
max_pids or from cgroups) is risky - OS might be doing some work and
interfere with the test.  Instead leave some reserve (hard-coded to
500) for the OS so the test won't fail on fork failure.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 lib/tst_pid.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/tst_pid.c b/lib/tst_pid.c
index c1ea3fe90e83..1b2e85959430 100644
--- a/lib/tst_pid.c
+++ b/lib/tst_pid.c
@@ -32,6 +32,8 @@
 #define PID_MAX_PATH "/proc/sys/kernel/pid_max"
 #define CGROUPS_V1_SLICE_FMT "/sys/fs/cgroup/pids/user.slice/user-%d.slice/pids.max"
 #define CGROUPS_V2_SLICE_FMT "/sys/fs/cgroup/user.slice/user-%d.slice/pids.max"
+/* Leave some available processes for the OS */
+#define PIDS_RESERVE 500
 
 pid_t tst_get_unused_pid_(void (*cleanup_fn) (void))
 {
@@ -106,7 +108,7 @@ static int get_session_pids_limit(void (*cleanup_fn) (void))
 	if (max_pids < 0)
 		return -1;
 
-	return max_pids;
+	return max_pids > PIDS_RESERVE ? max_pids - PIDS_RESERVE : 0;
 }
 
 int tst_get_free_pids_(void (*cleanup_fn) (void))
-- 
2.27.0