[LTP] [PATCH v2 0/2] swap{on,off} fixes for page size > 4KB

[LTP] [PATCH v2 0/2] swap{on,off} fixes for page size > 4KB

[Petr Vorel]

Hi Cyril, Li,

Changes v1->v2:
* Use 1MB for minimal swap file instead of using disk block size as a
  base measure of size (Cyril)

Kind regards,
Petr

Petr Vorel (2):
  libswap: Add {SAFE_,}MAKE_MINIMAL_SWAPFILE() macros
  libswap: Use {SAFE_,}MAKE_MINIMAL_SWAPFILE()

 include/libswap.h                             | 18 ++++++++++++++++++
 libs/libltpswap/libswap.c                     |  2 +-
 testcases/kernel/syscalls/swapoff/swapoff02.c |  2 +-
 testcases/kernel/syscalls/swapon/swapon02.c   |  4 ++--
 testcases/kernel/syscalls/swapon/swapon03.c   |  4 ++--
 5 files changed, 24 insertions(+), 6 deletions(-)

-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Petr Vorel]

65536 bytes triggered warning on systems with 64kb page size (e.g. on
aarch64 with CONFIG_ARM64_64K_PAGES=y or on ppc64le with
CONFIG_PAGE_SIZE_64KB=y):

    TWARN: Swapfile size is less than the system page size. Using page size
    (65536 bytes) instead of block size (4096 bytes).

1 MB should be ok for most of the systems.

Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Changes v1->v2:
* Use 1MB for minimal swap file instead of using disk block size as a
  base measure of size (Cyril)

 include/libswap.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/libswap.h b/include/libswap.h
index 87e32328e..28ce7842d 100644
--- a/include/libswap.h
+++ b/include/libswap.h
@@ -23,6 +23,24 @@ int make_swapfile(const char *file, const int lineno,
 			const char *swapfile, unsigned int num,
 			int safe, enum swapfile_method method);
 
+/* 65536 bytes is minimum for 64kb page size, let's use 1 MB */
+#define MINIMAL_SWAP_SIZE_MB 1
+
+/**
+ * Macro to create minimal swapfile.
+ */
+#define MAKE_MINIMAL_SWAPFILE(swapfile) \
+    make_swapfile(__FILE__, __LINE__, swapfile, MINIMAL_SWAP_SIZE_MB, 0, \
+		  SWAPFILE_BY_SIZE)
+
+/**
+ * Macro to create minimal swapfile.
+ * Includes safety checks to handle potential errors.
+ */
+#define SAFE_MAKE_MINIMAL_SWAPFILE(swapfile) \
+    make_swapfile(__FILE__, __LINE__, swapfile, MINIMAL_SWAP_SIZE_MB, 1, \
+		  SWAPFILE_BY_SIZE)
+
 /**
  * Macro to create swapfile size in megabytes (MB).
  */
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Li Wang]

Hi Petr, Cyril,

The patchset generally looks good.

But I have a concern about the macro name "MINIMAL" word,
which misled people to think that is the minimal swapfile size we
can make on the system, but obviously it is not, we could even
create a smaller one, right?

Can we rename it with a better one?


On Fri, Apr 19, 2024 at 2:52 AM Petr Vorel <pvorel@suse.cz> wrote:

> 65536 bytes triggered warning on systems with 64kb page size (e.g. on
> aarch64 with CONFIG_ARM64_64K_PAGES=y or on ppc64le with
> CONFIG_PAGE_SIZE_64KB=y):
>
>     TWARN: Swapfile size is less than the system page size. Using page size
>     (65536 bytes) instead of block size (4096 bytes).
>
> 1 MB should be ok for most of the systems.
>
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Changes v1->v2:
> * Use 1MB for minimal swap file instead of using disk block size as a
>   base measure of size (Cyril)
>
>  include/libswap.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/include/libswap.h b/include/libswap.h
> index 87e32328e..28ce7842d 100644
> --- a/include/libswap.h
> +++ b/include/libswap.h
> @@ -23,6 +23,24 @@ int make_swapfile(const char *file, const int lineno,
>                         const char *swapfile, unsigned int num,
>                         int safe, enum swapfile_method method);
>
> +/* 65536 bytes is minimum for 64kb page size, let's use 1 MB */
> +#define MINIMAL_SWAP_SIZE_MB 1
> +
> +/**
> + * Macro to create minimal swapfile.
> + */
> +#define MAKE_MINIMAL_SWAPFILE(swapfile) \
> +    make_swapfile(__FILE__, __LINE__, swapfile, MINIMAL_SWAP_SIZE_MB, 0, \
> +                 SWAPFILE_BY_SIZE)
> +
> +/**
> + * Macro to create minimal swapfile.
> + * Includes safety checks to handle potential errors.
> + */
> +#define SAFE_MAKE_MINIMAL_SWAPFILE(swapfile) \
> +    make_swapfile(__FILE__, __LINE__, swapfile, MINIMAL_SWAP_SIZE_MB, 1, \
> +                 SWAPFILE_BY_SIZE)
> +
>  /**
>   * Macro to create swapfile size in megabytes (MB).
>   */
> --
> 2.43.0
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Li Wang]

On Fri, Apr 19, 2024 at 9:50 AM Li Wang <liwang@redhat.com> wrote:

> Hi Petr, Cyril,
>
> The patchset generally looks good.
>
> But I have a concern about the macro name "MINIMAL" word,
> which misled people to think that is the minimal swapfile size we
> can make on the system, but obviously it is not, we could even
> create a smaller one, right?
>
> Can we rename it with a better one?
>

What about MAKE_DEFAULT_SWAPFILE, or MAKE_TEST_SWAPFILE?

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Petr Vorel]

> On Fri, Apr 19, 2024 at 9:50 AM Li Wang <liwang@redhat.com> wrote:

> > Hi Petr, Cyril,

> > The patchset generally looks good.

> > But I have a concern about the macro name "MINIMAL" word,
> > which misled people to think that is the minimal swapfile size we
> > can make on the system, but obviously it is not, we could even
> > create a smaller one, right?

> > Can we rename it with a better one?


> What about MAKE_DEFAULT_SWAPFILE, or MAKE_TEST_SWAPFILE?

I want to somehow express that it's a really small swap file
(although sure, not minimal). Sure, it can be "default" or "test",
but it does not say anything about the size.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Li Wang]

On Fri, Apr 19, 2024 at 2:12 PM Petr Vorel <pvorel@suse.cz> wrote:

> > On Fri, Apr 19, 2024 at 9:50 AM Li Wang <liwang@redhat.com> wrote:
>
> > > Hi Petr, Cyril,
>
> > > The patchset generally looks good.
>
> > > But I have a concern about the macro name "MINIMAL" word,
> > > which misled people to think that is the minimal swapfile size we
> > > can make on the system, but obviously it is not, we could even
> > > create a smaller one, right?
>
> > > Can we rename it with a better one?
>
>
> > What about MAKE_DEFAULT_SWAPFILE, or MAKE_TEST_SWAPFILE?
>
> I want to somehow express that it's a really small swap file
> (although sure, not minimal). Sure, it can be "default" or "test",
> but it does not say anything about the size.
>

Why do we have to emphasize the "small" swap file?
If we choose to use of "default" 1MB for LTP test but not
explicitly declarant in the name, that's okay, people can
check the defined value if they are interested.



>
> Kind regards,
> Petr
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Petr Vorel]

> On Fri, Apr 19, 2024 at 2:12 PM Petr Vorel <pvorel@suse.cz> wrote:

> > > On Fri, Apr 19, 2024 at 9:50 AM Li Wang <liwang@redhat.com> wrote:

> > > > Hi Petr, Cyril,

> > > > The patchset generally looks good.

> > > > But I have a concern about the macro name "MINIMAL" word,
> > > > which misled people to think that is the minimal swapfile size we
> > > > can make on the system, but obviously it is not, we could even
> > > > create a smaller one, right?

> > > > Can we rename it with a better one?


> > > What about MAKE_DEFAULT_SWAPFILE, or MAKE_TEST_SWAPFILE?

> > I want to somehow express that it's a really small swap file
> > (although sure, not minimal). Sure, it can be "default" or "test",
> > but it does not say anything about the size.


> Why do we have to emphasize the "small" swap file?

To make obvious on a first look that we are testing something which is not
typical use case (who creates 1MB swap file in reality?).

We have 5 swap tests: 2x not that big but it could be real swap usage (65536
blocks ~ 262 MB when 4kb and 128 MB) and the rest is 5x that 10 blocks, which
would be now changed 1 MB. Therefore most of them are small, not really
realistic size. Therefore it'd be good to make it obvious already from the test
source.

Kind regards,
Petr

> If we choose to use of "default" 1MB for LTP test but not
> explicitly declarant in the name, that's okay, people can
> check the defined value if they are interested.




> > Kind regards,
> > Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] libswap: Add {SAFE_, }MAKE_MINIMAL_SWAPFILE() macros

[Cyril Hrubis]

Hi!
> > What about MAKE_DEFAULT_SWAPFILE, or MAKE_TEST_SWAPFILE?
> 
> I want to somehow express that it's a really small swap file
> (although sure, not minimal). Sure, it can be "default" or "test",
> but it does not say anything about the size.

So maybe just use the word small and make it MAKE_SMALL_SWAPFILE() ?

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/2] libswap: Use {SAFE_,}MAKE_MINIMAL_SWAPFILE()

[Petr Vorel]

This effectively increases the minimal used number of blocks to 256.

All {SAFE_,}MAKE_SWAPFILE_SIZE() calls which were creating swap used 10
blocks. While this is ok on 4kb page size, it's too low on systems with
64kb page size (e.g. on aarch64 with CONFIG_ARM64_64K_PAGES=y or on
ppc64le with CONFIG_PAGE_SIZE_64KB=y):

    TWARN: Swapfile size is less than the system page size. Using page size
    (65536 bytes) instead of block size (4096 bytes).

Obviously it would fail also on kernels with CONFIG_PAGE_SIZE_256KB.

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
The same as in v1.

 libs/libltpswap/libswap.c                     | 2 +-
 testcases/kernel/syscalls/swapoff/swapoff02.c | 2 +-
 testcases/kernel/syscalls/swapon/swapon02.c   | 4 ++--
 testcases/kernel/syscalls/swapon/swapon03.c   | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/libs/libltpswap/libswap.c b/libs/libltpswap/libswap.c
index aed76dfe2..92127d8cd 100644
--- a/libs/libltpswap/libswap.c
+++ b/libs/libltpswap/libswap.c
@@ -192,7 +192,7 @@ int make_swapfile(const char *file, const int lineno,
 bool is_swap_supported(const char *filename)
 {
 	int i, sw_support = 0;
-	int ret = SAFE_MAKE_SWAPFILE_BLKS(filename, 10);
+	int ret = SAFE_MAKE_MINIMAL_SWAPFILE(filename);
 	int fi_contiguous = file_is_contiguous(filename);
 	long fs_type = tst_fs_type(filename);
 	const char *fstype = tst_fs_type_name(fs_type);
diff --git a/testcases/kernel/syscalls/swapoff/swapoff02.c b/testcases/kernel/syscalls/swapoff/swapoff02.c
index 5a15826e4..676d06619 100644
--- a/testcases/kernel/syscalls/swapoff/swapoff02.c
+++ b/testcases/kernel/syscalls/swapoff/swapoff02.c
@@ -87,7 +87,7 @@ static void setup(void)
 	nobody_uid = nobody->pw_uid;
 
 	is_swap_supported(TEST_FILE);
-	SAFE_MAKE_SWAPFILE_BLKS(SWAP_FILE, 10);
+	SAFE_MAKE_MINIMAL_SWAPFILE(SWAP_FILE);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/syscalls/swapon/swapon02.c b/testcases/kernel/syscalls/swapon/swapon02.c
index e5e29b8e7..20f3cfd26 100644
--- a/testcases/kernel/syscalls/swapon/swapon02.c
+++ b/testcases/kernel/syscalls/swapon/swapon02.c
@@ -50,8 +50,8 @@ static void setup(void)
 	is_swap_supported(TEST_FILE);
 
 	SAFE_TOUCH(NOTSWAP_FILE, 0777, NULL);
-	MAKE_SWAPFILE_BLKS(SWAP_FILE, 10);
-	MAKE_SWAPFILE_BLKS(USED_FILE, 10);
+	MAKE_MINIMAL_SWAPFILE(SWAP_FILE);
+	MAKE_MINIMAL_SWAPFILE(USED_FILE);
 
 	if (tst_syscall(__NR_swapon, USED_FILE, 0))
 		tst_res(TWARN | TERRNO, "swapon(alreadyused) failed");
diff --git a/testcases/kernel/syscalls/swapon/swapon03.c b/testcases/kernel/syscalls/swapon/swapon03.c
index 5295a6a73..342b5e38a 100644
--- a/testcases/kernel/syscalls/swapon/swapon03.c
+++ b/testcases/kernel/syscalls/swapon/swapon03.c
@@ -49,7 +49,7 @@ static int setup_swap(void)
 
 			/* Create the swapfile */
 			snprintf(filename, sizeof(filename), "%s%02d", TEST_FILE, j + 2);
-			MAKE_SWAPFILE_BLKS(filename, 10);
+			MAKE_MINIMAL_SWAPFILE(filename);
 
 			/* turn on the swap file */
 			TST_EXP_PASS_SILENT(swapon(filename, 0));
@@ -62,7 +62,7 @@ static int setup_swap(void)
 		tst_brk(TFAIL, "Failed to setup swap files");
 
 	tst_res(TINFO, "Successfully created %d swap files", swapfiles);
-	MAKE_SWAPFILE_BLKS(TEST_FILE, 10);
+	MAKE_MINIMAL_SWAPFILE(TEST_FILE);
 
 	return 0;
 }
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp