On Tue, Jan 30, 2024 at 10:31:24AM -0800, Nhat Pham wrote:
On Mon, Jan 29, 2024 at 5:24 PM Yosry Ahmed yosryahmed@google.com wrote:
[..]
-static int allocate_bytes(const char *cgroup, void *arg) +static int allocate_bytes_and_read(const char *cgroup, void *arg, bool read) { size_t size = (size_t)arg; char *mem = (char *)malloc(size);
int ret = 0; if (!mem) return -1; for (int i = 0; i < size; i += 4095) mem[i] = 'a';if (read) {/* cycle through the allocated memory to (z)swap in and out pages */for (int t = 0; t < 5; t++) {What benefit does the iteration serve here? I would guess one iteration is enough to swap everything in at least once, no?
There might be data races etc. that might not appear in one iteration. Running multiple iterations increases the probability of these bugs cropping up.
Hmm this is a test running in a single process, and I assume the rest of the system would be idle (at least from a zswap perspective). Did the iterations actually catch problems in this scenario (not specifically in this test, but generally in similar testing)?
Admittedly, the same effect could, perhaps, also be achieved by running the same test multiple times, so this is not a hill I will die on :) This is just a bit more convenient - CI infra often runs these tests once every time a new kernel is built.
[..]
+static int test_swapin(const char *root) +{
return test_zswapin_size(root, "0");+}
Why are we testing the no zswap case? I am all for testing but it seems out of scope here. It would have been understandable if we are testing memory.zswap.max itself, but we are not doing that.
Eh it's just by convenience. We already have the workload - any test for zswap can pretty much be turned into a test for swap by disabling zswap (and enabling swap), so I was trying to kill two birds with one stone and cover a bit more of the codebase.
We can check that no data is actually in zswap after test_zswapin_size(root, "0"), in which case it becomes more of a zswap test and we get a sanity check for memory.zswap.max == 0. WDYT?
Perhaps we can rename it to test_swpain_nozswap() or so.
FWIW, I think the tests here should really be separated from cgroup tests, but I understand why they were added here. There is a lot of testing for memcg interface and control for zswap, and a lot of nice helpers present.
Yeah FWIW, I agree :) I wonder if there's an easy way to inherit helpers from one test suite to another. Some sort of kselftest dependency? Or maybe move these cgroup helpers up the hierarchy (so that it can be shared by multiple selftest suites).
I am not fluent in kselftest so I can't claim to know the answer here. There are a lot of things to do testing-wise for zswap, but I am not asking anyone to do it because I don't have the time to do it myself. It would be nice though :)