| Message ID | 20220120150421.26639-1-andrea.cervesato@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] Reduce needed memory by the dio_read test | expand |
Hi, Acked-by: Martin Doucha <mdoucha@suse.cz> On 20. 01. 22 16:04, Andrea Cervesato wrote: > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de> > --- > runtest/ltp-aiodio.part4 | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4 > index fc1e1d479..8c10f67fb 100644 > --- a/runtest/ltp-aiodio.part4 > +++ b/runtest/ltp-aiodio.part4 > @@ -62,7 +62,7 @@ DIT002 dio_truncate > #Running read_checkzero > #gread_checkzero > #Running dio_read > -DOR000 dio_read -n 1 -i 100 > -DOR001 dio_read -n 10 -i 30 > -DOR002 dio_read -n 20 -i 15 > -DOR003 dio_read -n 100 -i 4 > +DOR000 dio_read -n 1 -i 100 -r 512k -w 512k -s 5M > +DOR001 dio_read -n 10 -i 30 -r 512k -w 512k -s 5M > +DOR002 dio_read -n 20 -i 15 -r 512k -w 512k -s 5M > +DOR003 dio_read -n 100 -i 4 -r 512k -w 512k -s 5M
Hi! > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de> > --- > runtest/ltp-aiodio.part4 | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4 > index fc1e1d479..8c10f67fb 100644 > --- a/runtest/ltp-aiodio.part4 > +++ b/runtest/ltp-aiodio.part4 > @@ -62,7 +62,7 @@ DIT002 dio_truncate > #Running read_checkzero > #gread_checkzero > #Running dio_read > -DOR000 dio_read -n 1 -i 100 > -DOR001 dio_read -n 10 -i 30 > -DOR002 dio_read -n 20 -i 15 > -DOR003 dio_read -n 100 -i 4 > +DOR000 dio_read -n 1 -i 100 -r 512k -w 512k -s 5M > +DOR001 dio_read -n 10 -i 30 -r 512k -w 512k -s 5M > +DOR002 dio_read -n 20 -i 15 -r 512k -w 512k -s 5M > +DOR003 dio_read -n 100 -i 4 -r 512k -w 512k -s 5M Limiting the child buffer to 512k makes sense, at least in the cases where n > 1 as by default the child allocates 32MB buffer, with 100 children that makes up to ~3GB. Also this really should have been in the patch description, which should really describe why the changes have been done. But I'm not sure about the -s 5M part, the file size was always 128MB before and reducing it to 5M sounds like too much. On my machine that reduces the testing time from about one minute to a few seconds, that sounds like change that is too radical without further explanation. If we want to run the test with smaller file size I would go for 64M, that reduces the runtime to half without reducing the changes for triggering a bug too much.
Hi all, > Hi! > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de> > > --- > > runtest/ltp-aiodio.part4 | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4 > > index fc1e1d479..8c10f67fb 100644 > > --- a/runtest/ltp-aiodio.part4 > > +++ b/runtest/ltp-aiodio.part4 > > @@ -62,7 +62,7 @@ DIT002 dio_truncate > > #Running read_checkzero > > #gread_checkzero > > #Running dio_read > > -DOR000 dio_read -n 1 -i 100 > > -DOR001 dio_read -n 10 -i 30 > > -DOR002 dio_read -n 20 -i 15 > > -DOR003 dio_read -n 100 -i 4 > > +DOR000 dio_read -n 1 -i 100 -r 512k -w 512k -s 5M > > +DOR001 dio_read -n 10 -i 30 -r 512k -w 512k -s 5M > > +DOR002 dio_read -n 20 -i 15 -r 512k -w 512k -s 5M > > +DOR003 dio_read -n 100 -i 4 -r 512k -w 512k -s 5M > Limiting the child buffer to 512k makes sense, at least in the cases > where n > 1 as by default the child allocates 32MB buffer, with 100 > children that makes up to ~3GB. Also this really should have been in the > patch description, which should really describe why the changes have > been done. > But I'm not sure about the -s 5M part, the file size was always 128MB > before and reducing it to 5M sounds like too much. On my machine that > reduces the testing time from about one minute to a few seconds, that sounds > like change that is too radical without further explanation. If we want > to run the test with smaller file size I would go for 64M, that reduces > the runtime to half without reducing the changes for triggering a bug > too much. Good catch. I'm also going to verify ADSP075 dio_sparse -w 518192k -s 518192k -n 1000 from ltp-aiodio.part2. Cyril's suggestion is to use '-n 1' to emulate only single children in previous implementation. Kind regards, Petr
Hi! By changing the file size to 32M, test requires 2 minutes to complete on my machine using ~51MB memory allocation for 100 threads. We can go for it. I'll also write a little explanation to the patch. Andrea On 1/20/22 16:31, Cyril Hrubis wrote: > Hi! >> Signed-off-by: Andrea Cervesato<andrea.cervesato@suse.de> >> --- >> runtest/ltp-aiodio.part4 | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4 >> index fc1e1d479..8c10f67fb 100644 >> --- a/runtest/ltp-aiodio.part4 >> +++ b/runtest/ltp-aiodio.part4 >> @@ -62,7 +62,7 @@ DIT002 dio_truncate >> #Running read_checkzero >> #gread_checkzero >> #Running dio_read >> -DOR000 dio_read -n 1 -i 100 >> -DOR001 dio_read -n 10 -i 30 >> -DOR002 dio_read -n 20 -i 15 >> -DOR003 dio_read -n 100 -i 4 >> +DOR000 dio_read -n 1 -i 100 -r 512k -w 512k -s 5M >> +DOR001 dio_read -n 10 -i 30 -r 512k -w 512k -s 5M >> +DOR002 dio_read -n 20 -i 15 -r 512k -w 512k -s 5M >> +DOR003 dio_read -n 100 -i 4 -r 512k -w 512k -s 5M > Limiting the child buffer to 512k makes sense, at least in the cases > where n > 1 as by default the child allocates 32MB buffer, with 100 > children that makes up to ~3GB. Also this really should have been in the > patch description, which should really describe why the changes have > been done. > > But I'm not sure about the -s 5M part, the file size was always 128MB > before and reducing it to 5M sounds like too much. On my machine that > reduces the testing time from about one minute to a few seconds, that sounds > like change that is too radical without further explanation. If we want > to run the test with smaller file size I would go for 64M, that reduces > the runtime to half without reducing the changes for triggering a bug > too much. >
Hi all, ... > > But I'm not sure about the -s 5M part, the file size was always 128MB > > before and reducing it to 5M sounds like too much. On my machine that > > reduces the testing time from about one minute to a few seconds, that sounds > > like change that is too radical without further explanation. If we want > > to run the test with smaller file size I would go for 64M, that reduces > > the runtime to half without reducing the changes for triggering a bug > > too much. > Good catch. > I'm also going to verify ADSP075 dio_sparse -w 518192k -s 518192k -n 1000 > from ltp-aiodio.part2. Cyril's suggestion is to use '-n 1' to emulate > only single children in previous implementation. No, this is wrong. Also the issue can be just a timeout. ADSP074 (uses -n 512) runs framework ~3.5 min, ADSP075 (uses -n 1000) gets killed by LTP timeout. Do we want to increase tst_test.timeout for test? Am I correct that we cannot increase based on particular input (for particular getopt value) as we cannot later update results.timeout during run? Kind regards, Petr > Kind regards, > Petr
Hi! > No, this is wrong. Also the issue can be just a timeout. > ADSP074 (uses -n 512) runs framework ~3.5 min, ADSP075 (uses -n 1000) gets killed > by LTP timeout. Do we want to increase tst_test.timeout for test? > > Am I correct that we cannot increase based on particular input (for particular > getopt value) as we cannot later update results.timeout during run? There actually is tst_set_timeout() however in this case I wouldn't bother because the test runtime largerly depends on the actuall speed of the disk it is writing to and other parameters of the test such as block size and the actual formula would be quite complex. Also I do plan to fix this properly by V2 of the runtime patchset once the release is finished, so I would go for minimal fix just before the release which would be setting tst_test.timeout to sufficiently large value.
Hi! > Also I do plan to fix this properly by V2 of the runtime patchset once > the release is finished, so I would go for minimal fix just before the > release which would be setting tst_test.timeout to sufficiently large > value. And I guess that we should do that for all the rewritten aio tests.
Hi Cyril, > Hi! > > Also I do plan to fix this properly by V2 of the runtime patchset once > > the release is finished, so I would go for minimal fix just before the > > release which would be setting tst_test.timeout to sufficiently large > > value. > And I guess that we should do that for all the rewritten aio tests. Sure, agree. Would be .timeout = 1800 enough (30 min)? I can send a patch based on my last patch "aiodio: Skip tests on tmpfs" [1] Kind regards, Petr [1] https://lore.kernel.org/ltp/YeqIB+Y4rO2XK57i@pevik/T/#t
diff --git a/runtest/ltp-aiodio.part4 b/runtest/ltp-aiodio.part4 index fc1e1d479..8c10f67fb 100644 --- a/runtest/ltp-aiodio.part4 +++ b/runtest/ltp-aiodio.part4 @@ -62,7 +62,7 @@ DIT002 dio_truncate #Running read_checkzero #gread_checkzero #Running dio_read -DOR000 dio_read -n 1 -i 100 -DOR001 dio_read -n 10 -i 30 -DOR002 dio_read -n 20 -i 15 -DOR003 dio_read -n 100 -i 4 +DOR000 dio_read -n 1 -i 100 -r 512k -w 512k -s 5M +DOR001 dio_read -n 10 -i 30 -r 512k -w 512k -s 5M +DOR002 dio_read -n 20 -i 15 -r 512k -w 512k -s 5M +DOR003 dio_read -n 100 -i 4 -r 512k -w 512k -s 5M
Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.de> --- runtest/ltp-aiodio.part4 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)