Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue Apr 2, 2024 at 8:40 PM EEST, Michal Koutný wrote: > On Tue, Apr 02, 2024 at 11:20:21AM -0500, Haitao Huang > wrote: > > Do we really want to have it implemented in c? > > I only pointed to the available C boilerplate. > > > There are much fewer lines of > > code in shell scripts. Note we are not really testing basic cgroup stuff. > > All we needed were creating/deleting cgroups and set limits which I think > > have been demonstrated feasible in the ash scripts now. > > I assume you refer to > Message-Id: <20240331174442.51019-1-haitao.hu...@linux.intel.com> > right? > > Could it be even simpler if you didn't stick to cgtools APIs and v1 > compatibility? > > Reducing ash_cgexec.sh to something like > echo 0 >$R/$1/cgroup.procs > shift > exec "$@" > (with some small builerplate for $R and previous mkdirs) I already asked about necessity of v1 in some response, and fully support this idea. Then cgexec can simply be a function wrapping along the lines what you proposed. BR, Jarkko
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue Apr 2, 2024 at 7:20 PM EEST, Haitao Huang wrote:
> On Tue, 02 Apr 2024 06:58:40 -0500, Jarkko Sakkinen
> wrote:
>
> > On Tue Apr 2, 2024 at 2:23 PM EEST, Michal Koutný wrote:
> >> Hello.
> >>
> >> On Sat, Mar 30, 2024 at 01:26:08PM +0200, Jarkko Sakkinen
> >> wrote:
> >> > > > It'd be more complicated and less readable to do all the stuff
> >> without the
> >> > > > cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only
> >> depends
> >> > > > on libc so I hope this would not cause too much inconvenience.
> >> > >
> >> > > As per cgroup-tools, please prove this. It makes the job for more
> >> > > complicated *for you* and you are making the job more complicated
> >> > > to every possible person in the planet running any kernel QA.
> >> > >
> >> > > I weight the latter more than the former. And it is exactly the
> >> > > reason why we did custom user space kselftest in the first place.
> >> > > Let's keep the tradition. All I can say is that kselftest is
> >> > > unfinished in its current form.
> >> > >
> >> > > What is "esp cgexec"?
> >> >
> >> > Also in kselftest we don't drive ultimate simplicity, we drive
> >> > efficient CI/QA. By open coding something like subset of
> >> > cgroup-tools needed to run the test you also help us later
> >> > on to backtrack the kernel changes. With cgroups-tools you
> >> > would have to use strace to get the same info.
> >>
> >> FWIW, see also functions in
> >> tools/testing/selftests/cgroup/cgroup_util.{h,c}.
> >> They likely cover what you need already -- if the tests are in C.
> >>
> >> (I admit that stuff in tools/testing/selftests/cgroup/ is best
> >> understood with strace.)
> >
> > Thanks!
> >
> > My conclusions are that:
> >
> > 1. We probably cannot move the test part of cgroup test itself
> >given the enclave payload dependency.
> > 2. I think it makes sense to still follow the same pattern as
> >other cgroups test and re-use cgroup_util.[ch] functionaltiy.
> >
> > So yeah I guess we need two test programs instead of one.
> >
> > Something along the lines:
> >
> > 1. main.[ch] -> test_sgx.[ch]
> > 2. introduce test_sgx_cgroup.c
> >
> > And test_sgx_cgroup.c would be implement similar test as the shell
> > script and would follow the structure of existing cgroups tests.
> >
> >>
> >> HTH,
> >> Michal
> >
> > BR, Jarkko
> >
> Do we really want to have it implemented in c? There are much fewer lines
> of code in shell scripts. Note we are not really testing basic cgroup
> stuff. All we needed were creating/deleting cgroups and set limits which I
> think have been demonstrated feasible in the ash scripts now.
>
> Given Dave's comments, and test scripts being working and cover the cases
> needed IMHO, I don't see much need to move to c code. I can add more cases
> if needed and fall back a c implementation later if any case can't be
> implemented in scripts. How about that?
We can settle to: ash + no dependencies. I guess you have for that
all the work done already.
BR, Jarkko
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue Apr 2, 2024 at 6:42 PM EEST, Dave Hansen wrote: > On 3/30/24 04:23, Jarkko Sakkinen wrote: > >>> I also wonder is cgroup-tools dependency absolutely required or could > >>> you just have a function that would interact with sysfs? > >> I should have checked email before hit the send button for v10 . > >> > >> It'd be more complicated and less readable to do all the stuff without the > >> > >> cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only depends > >> on libc so I hope this would not cause too much inconvenience. > > As per cgroup-tools, please prove this. It makes the job for more > > complicated *for you* and you are making the job more complicated > > to every possible person in the planet running any kernel QA. > > I don't see any other use of cgroup-tools in testing/selftests. > > I *DO* see a ton of /bin/bash use though. I wouldn't go to much trouble > to make the thing ash-compatible. > > That said, the most important thing is to get some selftests in place. > If using cgroup-tools means we get actual, runnable tests in place, > that's a heck of a lot more important than making them perfect. > Remember, almost nobody uses SGX. It's available on *VERY* few systems > from one CPU vendor and only in very specific hardware configurations. Ash-compatible is good enough for me, so let's draw the line there. Ash-compatibility does not cause any major hurdle as can we seen from Haitao's patch. Earlier version was not even POSIX-compatible, given that it used hard-coded path. Most of the added stuff come open coding the tools but in the test code that is not the big deal, and helps with debugging in the future. Even right now it helps reviewing kernel patches because it documents exactly how the feature is seen from user space. BR, Jarkko
Re: Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue, 02 Apr 2024 12:40:03 -0500, Michal Koutný wrote: On Tue, Apr 02, 2024 at 11:20:21AM -0500, Haitao Huang wrote: Do we really want to have it implemented in c? I only pointed to the available C boilerplate. There are much fewer lines of code in shell scripts. Note we are not really testing basic cgroup stuff. All we needed were creating/deleting cgroups and set limits which I think have been demonstrated feasible in the ash scripts now. I assume you refer to Message-Id: <20240331174442.51019-1-haitao.hu...@linux.intel.com> right? Could it be even simpler if you didn't stick to cgtools APIs and v1 compatibility? Reducing ash_cgexec.sh to something like echo 0 >$R/$1/cgroup.procs shift exec "$@" (with some small builerplate for $R and previous mkdirs) Yes, Thanks for the suggestion. Haitao
Re: Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue, Apr 02, 2024 at 11:20:21AM -0500, Haitao Huang wrote: > Do we really want to have it implemented in c? I only pointed to the available C boilerplate. > There are much fewer lines of > code in shell scripts. Note we are not really testing basic cgroup stuff. > All we needed were creating/deleting cgroups and set limits which I think > have been demonstrated feasible in the ash scripts now. I assume you refer to Message-Id: <20240331174442.51019-1-haitao.hu...@linux.intel.com> right? Could it be even simpler if you didn't stick to cgtools APIs and v1 compatibility? Reducing ash_cgexec.sh to something like echo 0 >$R/$1/cgroup.procs shift exec "$@" (with some small builerplate for $R and previous mkdirs) Thanks, Michal signature.asc Description: PGP signature
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue, 02 Apr 2024 06:58:40 -0500, Jarkko Sakkinen
wrote:
On Tue Apr 2, 2024 at 2:23 PM EEST, Michal Koutný wrote:
Hello.
On Sat, Mar 30, 2024 at 01:26:08PM +0200, Jarkko Sakkinen
wrote:
> > > It'd be more complicated and less readable to do all the stuff
without the
> > > cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only
depends
> > > on libc so I hope this would not cause too much inconvenience.
> >
> > As per cgroup-tools, please prove this. It makes the job for more
> > complicated *for you* and you are making the job more complicated
> > to every possible person in the planet running any kernel QA.
> >
> > I weight the latter more than the former. And it is exactly the
> > reason why we did custom user space kselftest in the first place.
> > Let's keep the tradition. All I can say is that kselftest is
> > unfinished in its current form.
> >
> > What is "esp cgexec"?
>
> Also in kselftest we don't drive ultimate simplicity, we drive
> efficient CI/QA. By open coding something like subset of
> cgroup-tools needed to run the test you also help us later
> on to backtrack the kernel changes. With cgroups-tools you
> would have to use strace to get the same info.
FWIW, see also functions in
tools/testing/selftests/cgroup/cgroup_util.{h,c}.
They likely cover what you need already -- if the tests are in C.
(I admit that stuff in tools/testing/selftests/cgroup/ is best
understood with strace.)
Thanks!
My conclusions are that:
1. We probably cannot move the test part of cgroup test itself
given the enclave payload dependency.
2. I think it makes sense to still follow the same pattern as
other cgroups test and re-use cgroup_util.[ch] functionaltiy.
So yeah I guess we need two test programs instead of one.
Something along the lines:
1. main.[ch] -> test_sgx.[ch]
2. introduce test_sgx_cgroup.c
And test_sgx_cgroup.c would be implement similar test as the shell
script and would follow the structure of existing cgroups tests.
HTH,
Michal
BR, Jarkko
Do we really want to have it implemented in c? There are much fewer lines
of code in shell scripts. Note we are not really testing basic cgroup
stuff. All we needed were creating/deleting cgroups and set limits which I
think have been demonstrated feasible in the ash scripts now.
Given Dave's comments, and test scripts being working and cover the cases
needed IMHO, I don't see much need to move to c code. I can add more cases
if needed and fall back a c implementation later if any case can't be
implemented in scripts. How about that?
Haitao
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On 3/30/24 04:23, Jarkko Sakkinen wrote: >>> I also wonder is cgroup-tools dependency absolutely required or could >>> you just have a function that would interact with sysfs? >> I should have checked email before hit the send button for v10 . >> >> It'd be more complicated and less readable to do all the stuff without the >> cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only depends >> on libc so I hope this would not cause too much inconvenience. > As per cgroup-tools, please prove this. It makes the job for more > complicated *for you* and you are making the job more complicated > to every possible person in the planet running any kernel QA. I don't see any other use of cgroup-tools in testing/selftests. I *DO* see a ton of /bin/bash use though. I wouldn't go to much trouble to make the thing ash-compatible. That said, the most important thing is to get some selftests in place. If using cgroup-tools means we get actual, runnable tests in place, that's a heck of a lot more important than making them perfect. Remember, almost nobody uses SGX. It's available on *VERY* few systems from one CPU vendor and only in very specific hardware configurations.
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Tue Apr 2, 2024 at 2:23 PM EEST, Michal Koutný wrote:
> Hello.
>
> On Sat, Mar 30, 2024 at 01:26:08PM +0200, Jarkko Sakkinen
> wrote:
> > > > It'd be more complicated and less readable to do all the stuff without
> > > > the
> > > > cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only
> > > > depends
> > > > on libc so I hope this would not cause too much inconvenience.
> > >
> > > As per cgroup-tools, please prove this. It makes the job for more
> > > complicated *for you* and you are making the job more complicated
> > > to every possible person in the planet running any kernel QA.
> > >
> > > I weight the latter more than the former. And it is exactly the
> > > reason why we did custom user space kselftest in the first place.
> > > Let's keep the tradition. All I can say is that kselftest is
> > > unfinished in its current form.
> > >
> > > What is "esp cgexec"?
> >
> > Also in kselftest we don't drive ultimate simplicity, we drive
> > efficient CI/QA. By open coding something like subset of
> > cgroup-tools needed to run the test you also help us later
> > on to backtrack the kernel changes. With cgroups-tools you
> > would have to use strace to get the same info.
>
> FWIW, see also functions in
> tools/testing/selftests/cgroup/cgroup_util.{h,c}.
> They likely cover what you need already -- if the tests are in C.
>
> (I admit that stuff in tools/testing/selftests/cgroup/ is best
> understood with strace.)
Thanks!
My conclusions are that:
1. We probably cannot move the test part of cgroup test itself
given the enclave payload dependency.
2. I think it makes sense to still follow the same pattern as
other cgroups test and re-use cgroup_util.[ch] functionaltiy.
So yeah I guess we need two test programs instead of one.
Something along the lines:
1. main.[ch] -> test_sgx.[ch]
2. introduce test_sgx_cgroup.c
And test_sgx_cgroup.c would be implement similar test as the shell
script and would follow the structure of existing cgroups tests.
>
> HTH,
> Michal
BR, Jarkko
Re: Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
Hello.
On Sat, Mar 30, 2024 at 01:26:08PM +0200, Jarkko Sakkinen
wrote:
> > > It'd be more complicated and less readable to do all the stuff without
> > > the
> > > cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only depends
> > >
> > > on libc so I hope this would not cause too much inconvenience.
> >
> > As per cgroup-tools, please prove this. It makes the job for more
> > complicated *for you* and you are making the job more complicated
> > to every possible person in the planet running any kernel QA.
> >
> > I weight the latter more than the former. And it is exactly the
> > reason why we did custom user space kselftest in the first place.
> > Let's keep the tradition. All I can say is that kselftest is
> > unfinished in its current form.
> >
> > What is "esp cgexec"?
>
> Also in kselftest we don't drive ultimate simplicity, we drive
> efficient CI/QA. By open coding something like subset of
> cgroup-tools needed to run the test you also help us later
> on to backtrack the kernel changes. With cgroups-tools you
> would have to use strace to get the same info.
FWIW, see also functions in
tools/testing/selftests/cgroup/cgroup_util.{h,c}.
They likely cover what you need already -- if the tests are in C.
(I admit that stuff in tools/testing/selftests/cgroup/ is best
understood with strace.)
HTH,
Michal
signature.asc
Description: PGP signature
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Sun Mar 31, 2024 at 8:35 PM EEST, Haitao Huang wrote: > On Sun, 31 Mar 2024 11:19:04 -0500, Jarkko Sakkinen > wrote: > > > On Sat Mar 30, 2024 at 5:32 PM EET, Haitao Huang wrote: > >> On Sat, 30 Mar 2024 06:15:14 -0500, Jarkko Sakkinen > >> wrote: > >> > >> > On Thu Mar 28, 2024 at 5:54 AM EET, Haitao Huang wrote: > >> >> On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen > >> > >> >> wrote: > >> >> > >> >> > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > >> >> >> The scripts rely on cgroup-tools package from libcgroup [1]. > >> >> >> > >> >> >> To run selftests for epc cgroup: > >> >> >> > >> >> >> sudo ./run_epc_cg_selftests.sh > >> >> >> > >> >> >> To watch misc cgroup 'current' changes during testing, run this > >> in a > >> >> >> separate terminal: > >> >> >> > >> >> >> ./watch_misc_for_tests.sh current > >> >> >> > >> >> >> With different cgroups, the script starts one or multiple > >> concurrent > >> >> >> SGX > >> >> >> selftests, each to run one unclobbered_vdso_oversubscribed > >> test.Each > >> >> >> of such test tries to load an enclave of EPC size equal to the EPC > >> >> >> capacity available on the platform. The script checks results > >> against > >> >> >> the expectation set for each cgroup and reports success or > >> failure. > >> >> >> > >> >> >> The script creates 3 different cgroups at the beginning with > >> >> >> following > >> >> >> expectations: > >> >> >> > >> >> >> 1) SMALL - intentionally small enough to fail the test loading an > >> >> >> enclave of size equal to the capacity. > >> >> >> 2) LARGE - large enough to run up to 4 concurrent tests but fail > >> some > >> >> >> if > >> >> >> more than 4 concurrent tests are run. The script starts 4 > >> expecting > >> >> >> at > >> >> >> least one test to pass, and then starts 5 expecting at least one > >> test > >> >> >> to fail. > >> >> >> 3) LARGER - limit is the same as the capacity, large enough to run > >> >> >> lots of > >> >> >> concurrent tests. The script starts 8 of them and expects all > >> pass. > >> >> >> Then it reruns the same test with one process randomly killed and > >> >> >> usage checked to be zero after all process exit. > >> >> >> > >> >> >> The script also includes a test with low mem_cg limit and LARGE > >> >> >> sgx_epc > >> >> >> limit to verify that the RAM used for per-cgroup reclamation is > >> >> >> charged > >> >> >> to a proper mem_cg. > >> >> >> > >> >> >> [1] https://github.com/libcgroup/libcgroup/blob/main/README > >> >> >> > >> >> >> Signed-off-by: Haitao Huang > >> >> >> --- > >> >> >> V7: > >> >> >> - Added memcontrol test. > >> >> >> > >> >> >> V5: > >> >> >> - Added script with automatic results checking, remove the > >> >> >> interactive > >> >> >> script. > >> >> >> - The script can run independent from the series below. > >> >> >> --- > >> >> >> .../selftests/sgx/run_epc_cg_selftests.sh | 246 > >> >> >> ++ > >> >> >> .../selftests/sgx/watch_misc_for_tests.sh | 13 + > >> >> >> 2 files changed, 259 insertions(+) > >> >> >> create mode 100755 > >> >> >> tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> >> create mode 100755 > >> >> >> tools/testing/selftests/sgx/watch_misc_for_tests.sh > >> >> >> > >> >> >> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> >> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> >> new file mode 100755 > >> >> >> index ..e027bf39f005 > >> >> >> --- /dev/null > >> >> >> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> >> @@ -0,0 +1,246 @@ > >> >> >> +#!/bin/bash > >> >> > > >> >> > This is not portable and neither does hold in the wild. > >> >> > > >> >> > It does not even often hold as it is not uncommon to place bash > >> >> > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > >> >> > a path that is neither of those two. > >> >> > > >> >> > Should be #!/usr/bin/env bash > >> >> > > >> >> > That is POSIX compatible form. > >> >> > > >> >> > >> >> Sure > >> >> > >> >> > Just got around trying to test this in NUC7 so looking into this in > >> >> > more detail. > >> >> > >> >> Thanks. Could you please check if this version works for you? > >> >> > >> >> > >> https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f > >> >> > >> >> > > >> >> > That said can you make the script work with just "#!/usr/bin/env > >> sh" > >> >> > and make sure that it is busybox ash compatible? > >> >> > >> >> Yes. > >> >> > >> >> > > >> >> > I don't see any necessity to make this bash only and it adds to the > >> >> > compilation time of the image. Otherwise lot of this could be > >> tested > >> >> > just with qemu+bzImage+busybox(inside initramfs). > >> >> > > >> >> > >> >> will still need cgroup-tools as you pointed out later. Compiling from > >> >> its > >> >> upstream code OK? > >> > > >> > Can you explain why you need it? > >> > > >> > What is the thing you cannot do without it? > >> > > >> >
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Sun, 31 Mar 2024 11:19:04 -0500, Jarkko Sakkinen wrote: On Sat Mar 30, 2024 at 5:32 PM EET, Haitao Huang wrote: On Sat, 30 Mar 2024 06:15:14 -0500, Jarkko Sakkinen wrote: > On Thu Mar 28, 2024 at 5:54 AM EET, Haitao Huang wrote: >> On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen >> wrote: >> >> > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: >> >> The scripts rely on cgroup-tools package from libcgroup [1]. >> >> >> >> To run selftests for epc cgroup: >> >> >> >> sudo ./run_epc_cg_selftests.sh >> >> >> >> To watch misc cgroup 'current' changes during testing, run this in a >> >> separate terminal: >> >> >> >> ./watch_misc_for_tests.sh current >> >> >> >> With different cgroups, the script starts one or multiple concurrent >> >> SGX >> >> selftests, each to run one unclobbered_vdso_oversubscribed test.Each >> >> of such test tries to load an enclave of EPC size equal to the EPC >> >> capacity available on the platform. The script checks results against >> >> the expectation set for each cgroup and reports success or failure. >> >> >> >> The script creates 3 different cgroups at the beginning with >> >> following >> >> expectations: >> >> >> >> 1) SMALL - intentionally small enough to fail the test loading an >> >> enclave of size equal to the capacity. >> >> 2) LARGE - large enough to run up to 4 concurrent tests but fail some >> >> if >> >> more than 4 concurrent tests are run. The script starts 4 expecting >> >> at >> >> least one test to pass, and then starts 5 expecting at least one test >> >> to fail. >> >> 3) LARGER - limit is the same as the capacity, large enough to run >> >> lots of >> >> concurrent tests. The script starts 8 of them and expects all pass. >> >> Then it reruns the same test with one process randomly killed and >> >> usage checked to be zero after all process exit. >> >> >> >> The script also includes a test with low mem_cg limit and LARGE >> >> sgx_epc >> >> limit to verify that the RAM used for per-cgroup reclamation is >> >> charged >> >> to a proper mem_cg. >> >> >> >> [1] https://github.com/libcgroup/libcgroup/blob/main/README >> >> >> >> Signed-off-by: Haitao Huang >> >> --- >> >> V7: >> >> - Added memcontrol test. >> >> >> >> V5: >> >> - Added script with automatic results checking, remove the >> >> interactive >> >> script. >> >> - The script can run independent from the series below. >> >> --- >> >> .../selftests/sgx/run_epc_cg_selftests.sh | 246 >> >> ++ >> >> .../selftests/sgx/watch_misc_for_tests.sh | 13 + >> >> 2 files changed, 259 insertions(+) >> >> create mode 100755 >> >> tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> >> create mode 100755 >> >> tools/testing/selftests/sgx/watch_misc_for_tests.sh >> >> >> >> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> >> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> >> new file mode 100755 >> >> index ..e027bf39f005 >> >> --- /dev/null >> >> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> >> @@ -0,0 +1,246 @@ >> >> +#!/bin/bash >> > >> > This is not portable and neither does hold in the wild. >> > >> > It does not even often hold as it is not uncommon to place bash >> > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has >> > a path that is neither of those two. >> > >> > Should be #!/usr/bin/env bash >> > >> > That is POSIX compatible form. >> > >> >> Sure >> >> > Just got around trying to test this in NUC7 so looking into this in >> > more detail. >> >> Thanks. Could you please check if this version works for you? >> >> https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f >> >> > >> > That said can you make the script work with just "#!/usr/bin/env sh" >> > and make sure that it is busybox ash compatible? >> >> Yes. >> >> > >> > I don't see any necessity to make this bash only and it adds to the >> > compilation time of the image. Otherwise lot of this could be tested >> > just with qemu+bzImage+busybox(inside initramfs). >> > >> >> will still need cgroup-tools as you pointed out later. Compiling from >> its >> upstream code OK? > > Can you explain why you need it? > > What is the thing you cannot do without it? > > BR, Jarkko > I did not find a nice way to start a process in a specified cgroup like cgexec does. I could wrap the test in a shell: move the current shell to a new cgroup then do exec to run the test app. But that seems cumbersome as I need to spawn many shells, and check results of them. Another option is create my own cgexec, which I'm sure will be very similar to cgexec code. Was not sure how far we want to go with this. I can experiment with the shell wrapper idea and see how bad it can be and fall back to implement cgexec? Open to suggestions. I guess there's only few variants of how cgexec is invoked right? The first thing we need to do is what exact steps those variants perform. After that
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Sat Mar 30, 2024 at 5:32 PM EET, Haitao Huang wrote: > On Sat, 30 Mar 2024 06:15:14 -0500, Jarkko Sakkinen > wrote: > > > On Thu Mar 28, 2024 at 5:54 AM EET, Haitao Huang wrote: > >> On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen > >> wrote: > >> > >> > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > >> >> The scripts rely on cgroup-tools package from libcgroup [1]. > >> >> > >> >> To run selftests for epc cgroup: > >> >> > >> >> sudo ./run_epc_cg_selftests.sh > >> >> > >> >> To watch misc cgroup 'current' changes during testing, run this in a > >> >> separate terminal: > >> >> > >> >> ./watch_misc_for_tests.sh current > >> >> > >> >> With different cgroups, the script starts one or multiple concurrent > >> >> SGX > >> >> selftests, each to run one unclobbered_vdso_oversubscribed test.Each > >> >> of such test tries to load an enclave of EPC size equal to the EPC > >> >> capacity available on the platform. The script checks results against > >> >> the expectation set for each cgroup and reports success or failure. > >> >> > >> >> The script creates 3 different cgroups at the beginning with > >> >> following > >> >> expectations: > >> >> > >> >> 1) SMALL - intentionally small enough to fail the test loading an > >> >> enclave of size equal to the capacity. > >> >> 2) LARGE - large enough to run up to 4 concurrent tests but fail some > >> >> if > >> >> more than 4 concurrent tests are run. The script starts 4 expecting > >> >> at > >> >> least one test to pass, and then starts 5 expecting at least one test > >> >> to fail. > >> >> 3) LARGER - limit is the same as the capacity, large enough to run > >> >> lots of > >> >> concurrent tests. The script starts 8 of them and expects all pass. > >> >> Then it reruns the same test with one process randomly killed and > >> >> usage checked to be zero after all process exit. > >> >> > >> >> The script also includes a test with low mem_cg limit and LARGE > >> >> sgx_epc > >> >> limit to verify that the RAM used for per-cgroup reclamation is > >> >> charged > >> >> to a proper mem_cg. > >> >> > >> >> [1] https://github.com/libcgroup/libcgroup/blob/main/README > >> >> > >> >> Signed-off-by: Haitao Huang > >> >> --- > >> >> V7: > >> >> - Added memcontrol test. > >> >> > >> >> V5: > >> >> - Added script with automatic results checking, remove the > >> >> interactive > >> >> script. > >> >> - The script can run independent from the series below. > >> >> --- > >> >> .../selftests/sgx/run_epc_cg_selftests.sh | 246 > >> >> ++ > >> >> .../selftests/sgx/watch_misc_for_tests.sh | 13 + > >> >> 2 files changed, 259 insertions(+) > >> >> create mode 100755 > >> >> tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> create mode 100755 > >> >> tools/testing/selftests/sgx/watch_misc_for_tests.sh > >> >> > >> >> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> new file mode 100755 > >> >> index ..e027bf39f005 > >> >> --- /dev/null > >> >> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> >> @@ -0,0 +1,246 @@ > >> >> +#!/bin/bash > >> > > >> > This is not portable and neither does hold in the wild. > >> > > >> > It does not even often hold as it is not uncommon to place bash > >> > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > >> > a path that is neither of those two. > >> > > >> > Should be #!/usr/bin/env bash > >> > > >> > That is POSIX compatible form. > >> > > >> > >> Sure > >> > >> > Just got around trying to test this in NUC7 so looking into this in > >> > more detail. > >> > >> Thanks. Could you please check if this version works for you? > >> > >> https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f > >> > >> > > >> > That said can you make the script work with just "#!/usr/bin/env sh" > >> > and make sure that it is busybox ash compatible? > >> > >> Yes. > >> > >> > > >> > I don't see any necessity to make this bash only and it adds to the > >> > compilation time of the image. Otherwise lot of this could be tested > >> > just with qemu+bzImage+busybox(inside initramfs). > >> > > >> > >> will still need cgroup-tools as you pointed out later. Compiling from > >> its > >> upstream code OK? > > > > Can you explain why you need it? > > > > What is the thing you cannot do without it? > > > > BR, Jarkko > > > I did not find a nice way to start a process in a specified cgroup like > cgexec does. I could wrap the test in a shell: move the current shell to a > new cgroup then do exec to run the test app. But that seems cumbersome as > I need to spawn many shells, and check results of them. Another option is > create my own cgexec, which I'm sure will be very similar to cgexec code. > Was not sure how far we want to go with this. > > I can experiment with the shell wrapper idea and see how bad it can be and > fall back to implement cgexec? Open to
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Sat, 30 Mar 2024 06:15:14 -0500, Jarkko Sakkinen wrote: On Thu Mar 28, 2024 at 5:54 AM EET, Haitao Huang wrote: On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen wrote: > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: >> The scripts rely on cgroup-tools package from libcgroup [1]. >> >> To run selftests for epc cgroup: >> >> sudo ./run_epc_cg_selftests.sh >> >> To watch misc cgroup 'current' changes during testing, run this in a >> separate terminal: >> >> ./watch_misc_for_tests.sh current >> >> With different cgroups, the script starts one or multiple concurrent >> SGX >> selftests, each to run one unclobbered_vdso_oversubscribed test.Each >> of such test tries to load an enclave of EPC size equal to the EPC >> capacity available on the platform. The script checks results against >> the expectation set for each cgroup and reports success or failure. >> >> The script creates 3 different cgroups at the beginning with >> following >> expectations: >> >> 1) SMALL - intentionally small enough to fail the test loading an >> enclave of size equal to the capacity. >> 2) LARGE - large enough to run up to 4 concurrent tests but fail some >> if >> more than 4 concurrent tests are run. The script starts 4 expecting >> at >> least one test to pass, and then starts 5 expecting at least one test >> to fail. >> 3) LARGER - limit is the same as the capacity, large enough to run >> lots of >> concurrent tests. The script starts 8 of them and expects all pass. >> Then it reruns the same test with one process randomly killed and >> usage checked to be zero after all process exit. >> >> The script also includes a test with low mem_cg limit and LARGE >> sgx_epc >> limit to verify that the RAM used for per-cgroup reclamation is >> charged >> to a proper mem_cg. >> >> [1] https://github.com/libcgroup/libcgroup/blob/main/README >> >> Signed-off-by: Haitao Huang >> --- >> V7: >> - Added memcontrol test. >> >> V5: >> - Added script with automatic results checking, remove the >> interactive >> script. >> - The script can run independent from the series below. >> --- >> .../selftests/sgx/run_epc_cg_selftests.sh | 246 >> ++ >> .../selftests/sgx/watch_misc_for_tests.sh | 13 + >> 2 files changed, 259 insertions(+) >> create mode 100755 >> tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> create mode 100755 >> tools/testing/selftests/sgx/watch_misc_for_tests.sh >> >> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> new file mode 100755 >> index ..e027bf39f005 >> --- /dev/null >> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh >> @@ -0,0 +1,246 @@ >> +#!/bin/bash > > This is not portable and neither does hold in the wild. > > It does not even often hold as it is not uncommon to place bash > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > a path that is neither of those two. > > Should be #!/usr/bin/env bash > > That is POSIX compatible form. > Sure > Just got around trying to test this in NUC7 so looking into this in > more detail. Thanks. Could you please check if this version works for you? https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f > > That said can you make the script work with just "#!/usr/bin/env sh" > and make sure that it is busybox ash compatible? Yes. > > I don't see any necessity to make this bash only and it adds to the > compilation time of the image. Otherwise lot of this could be tested > just with qemu+bzImage+busybox(inside initramfs). > will still need cgroup-tools as you pointed out later. Compiling from its upstream code OK? Can you explain why you need it? What is the thing you cannot do without it? BR, Jarkko I did not find a nice way to start a process in a specified cgroup like cgexec does. I could wrap the test in a shell: move the current shell to a new cgroup then do exec to run the test app. But that seems cumbersome as I need to spawn many shells, and check results of them. Another option is create my own cgexec, which I'm sure will be very similar to cgexec code. Was not sure how far we want to go with this. I can experiment with the shell wrapper idea and see how bad it can be and fall back to implement cgexec? Open to suggestions. Thanks Haitao
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Sat Mar 30, 2024 at 1:23 PM EET, Jarkko Sakkinen wrote: > On Thu Mar 28, 2024 at 2:57 AM EET, Haitao Huang wrote: > > On Wed, 27 Mar 2024 11:56:35 -0500, Jarkko Sakkinen > > wrote: > > > > > On Wed Mar 27, 2024 at 2:55 PM EET, Jarkko Sakkinen wrote: > > >> On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > > >> > The scripts rely on cgroup-tools package from libcgroup [1]. > > >> > > > >> > To run selftests for epc cgroup: > > >> > > > >> > sudo ./run_epc_cg_selftests.sh > > >> > > > >> > To watch misc cgroup 'current' changes during testing, run this in a > > >> > separate terminal: > > >> > > > >> > ./watch_misc_for_tests.sh current > > >> > > > >> > With different cgroups, the script starts one or multiple concurrent > > >> > SGX > > >> > selftests, each to run one unclobbered_vdso_oversubscribed test.> Each > > >> > of such test tries to load an enclave of EPC size equal to the EPC > > >> > capacity available on the platform. The script checks results against > > >> > the expectation set for each cgroup and reports success or failure. > > >> > > > >> > The script creates 3 different cgroups at the beginning with > > >> > following > > >> > expectations: > > >> > > > >> > 1) SMALL - intentionally small enough to fail the test loading an > > >> > enclave of size equal to the capacity. > > >> > 2) LARGE - large enough to run up to 4 concurrent tests but fail some > > >> > if > > >> > more than 4 concurrent tests are run. The script starts 4 expecting > > >> > at > > >> > least one test to pass, and then starts 5 expecting at least one test > > >> > to fail. > > >> > 3) LARGER - limit is the same as the capacity, large enough to run > > >> > lots of > > >> > concurrent tests. The script starts 8 of them and expects all pass. > > >> > Then it reruns the same test with one process randomly killed and > > >> > usage checked to be zero after all process exit. > > >> > > > >> > The script also includes a test with low mem_cg limit and LARGE > > >> > sgx_epc > > >> > limit to verify that the RAM used for per-cgroup reclamation is > > >> > charged > > >> > to a proper mem_cg. > > >> > > > >> > [1] https://github.com/libcgroup/libcgroup/blob/main/README > > >> > > > >> > Signed-off-by: Haitao Huang > > >> > --- > > >> > V7: > > >> > - Added memcontrol test. > > >> > > > >> > V5: > > >> > - Added script with automatic results checking, remove the > > >> > interactive > > >> > script. > > >> > - The script can run independent from the series below. > > >> > --- > > >> > .../selftests/sgx/run_epc_cg_selftests.sh | 246 > > >> > ++ > > >> > .../selftests/sgx/watch_misc_for_tests.sh | 13 + > > >> > 2 files changed, 259 insertions(+) > > >> > create mode 100755 > > >> > tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > >> > create mode 100755 > > >> > tools/testing/selftests/sgx/watch_misc_for_tests.sh > > >> > > > >> > diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > >> > b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > >> > new file mode 100755 > > >> > index ..e027bf39f005 > > >> > --- /dev/null > > >> > +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > >> > @@ -0,0 +1,246 @@ > > >> > +#!/bin/bash > > >> > > >> This is not portable and neither does hold in the wild. > > >> > > >> It does not even often hold as it is not uncommon to place bash > > >> to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > > >> a path that is neither of those two. > > >> > > >> Should be #!/usr/bin/env bash > > >> > > >> That is POSIX compatible form. > > >> > > >> Just got around trying to test this in NUC7 so looking into this in > > >> more detail. > > >> > > >> That said can you make the script work with just "#!/usr/bin/env sh" > > >> and make sure that it is busybox ash compatible? > > >> > > >> I don't see any necessity to make this bash only and it adds to the > > >> compilation time of the image. Otherwise lot of this could be tested > > >> just with qemu+bzImage+busybox(inside initramfs). > > >> > > >> Now you are adding fully glibc shenanigans for the sake of syntax > > >> sugar. > > >> > > >> > +# SPDX-License-Identifier: GPL-2.0 > > >> > +# Copyright(c) 2023 Intel Corporation. > > >> > + > > >> > +TEST_ROOT_CG=selftest > > >> > +cgcreate -g misc:$TEST_ROOT_CG > > >> > > >> How do you know that cgcreate exists? It is used a lot in the script > > >> with no check for the existence. Please fix e.g. with "command -v > > >> cgreate". > > >> > > >> > +if [ $? -ne 0 ]; then > > >> > +echo "# Please make sure cgroup-tools is installed, and misc > > >> > cgroup is mounted." > > >> > +exit 1 > > >> > +fi > > >> > > >> And please do not do it this way. Also, please remove the advice for > > >> "cgroups-tool". This is not meant to be debian only. Better would be > > >> to e.g. point out the URL of the upstream project. > > >> > > >> And yeah the whole message should be based on "command -v", not like > > >> this. >
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Thu Mar 28, 2024 at 2:57 AM EET, Haitao Huang wrote: > On Wed, 27 Mar 2024 11:56:35 -0500, Jarkko Sakkinen > wrote: > > > On Wed Mar 27, 2024 at 2:55 PM EET, Jarkko Sakkinen wrote: > >> On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > >> > The scripts rely on cgroup-tools package from libcgroup [1]. > >> > > >> > To run selftests for epc cgroup: > >> > > >> > sudo ./run_epc_cg_selftests.sh > >> > > >> > To watch misc cgroup 'current' changes during testing, run this in a > >> > separate terminal: > >> > > >> > ./watch_misc_for_tests.sh current > >> > > >> > With different cgroups, the script starts one or multiple concurrent > >> > SGX > >> > selftests, each to run one unclobbered_vdso_oversubscribed test.> Each > >> > of such test tries to load an enclave of EPC size equal to the EPC > >> > capacity available on the platform. The script checks results against > >> > the expectation set for each cgroup and reports success or failure. > >> > > >> > The script creates 3 different cgroups at the beginning with > >> > following > >> > expectations: > >> > > >> > 1) SMALL - intentionally small enough to fail the test loading an > >> > enclave of size equal to the capacity. > >> > 2) LARGE - large enough to run up to 4 concurrent tests but fail some > >> > if > >> > more than 4 concurrent tests are run. The script starts 4 expecting > >> > at > >> > least one test to pass, and then starts 5 expecting at least one test > >> > to fail. > >> > 3) LARGER - limit is the same as the capacity, large enough to run > >> > lots of > >> > concurrent tests. The script starts 8 of them and expects all pass. > >> > Then it reruns the same test with one process randomly killed and > >> > usage checked to be zero after all process exit. > >> > > >> > The script also includes a test with low mem_cg limit and LARGE > >> > sgx_epc > >> > limit to verify that the RAM used for per-cgroup reclamation is > >> > charged > >> > to a proper mem_cg. > >> > > >> > [1] https://github.com/libcgroup/libcgroup/blob/main/README > >> > > >> > Signed-off-by: Haitao Huang > >> > --- > >> > V7: > >> > - Added memcontrol test. > >> > > >> > V5: > >> > - Added script with automatic results checking, remove the > >> > interactive > >> > script. > >> > - The script can run independent from the series below. > >> > --- > >> > .../selftests/sgx/run_epc_cg_selftests.sh | 246 > >> > ++ > >> > .../selftests/sgx/watch_misc_for_tests.sh | 13 + > >> > 2 files changed, 259 insertions(+) > >> > create mode 100755 > >> > tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> > create mode 100755 > >> > tools/testing/selftests/sgx/watch_misc_for_tests.sh > >> > > >> > diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> > b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> > new file mode 100755 > >> > index ..e027bf39f005 > >> > --- /dev/null > >> > +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> > @@ -0,0 +1,246 @@ > >> > +#!/bin/bash > >> > >> This is not portable and neither does hold in the wild. > >> > >> It does not even often hold as it is not uncommon to place bash > >> to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > >> a path that is neither of those two. > >> > >> Should be #!/usr/bin/env bash > >> > >> That is POSIX compatible form. > >> > >> Just got around trying to test this in NUC7 so looking into this in > >> more detail. > >> > >> That said can you make the script work with just "#!/usr/bin/env sh" > >> and make sure that it is busybox ash compatible? > >> > >> I don't see any necessity to make this bash only and it adds to the > >> compilation time of the image. Otherwise lot of this could be tested > >> just with qemu+bzImage+busybox(inside initramfs). > >> > >> Now you are adding fully glibc shenanigans for the sake of syntax > >> sugar. > >> > >> > +# SPDX-License-Identifier: GPL-2.0 > >> > +# Copyright(c) 2023 Intel Corporation. > >> > + > >> > +TEST_ROOT_CG=selftest > >> > +cgcreate -g misc:$TEST_ROOT_CG > >> > >> How do you know that cgcreate exists? It is used a lot in the script > >> with no check for the existence. Please fix e.g. with "command -v > >> cgreate". > >> > >> > +if [ $? -ne 0 ]; then > >> > +echo "# Please make sure cgroup-tools is installed, and misc > >> > cgroup is mounted." > >> > +exit 1 > >> > +fi > >> > >> And please do not do it this way. Also, please remove the advice for > >> "cgroups-tool". This is not meant to be debian only. Better would be > >> to e.g. point out the URL of the upstream project. > >> > >> And yeah the whole message should be based on "command -v", not like > >> this. > >> > >> > +TEST_CG_SUB1=$TEST_ROOT_CG/test1 > >> > +TEST_CG_SUB2=$TEST_ROOT_CG/test2 > >> > +# We will only set limit in test1 and run tests in test3 > >> > +TEST_CG_SUB3=$TEST_ROOT_CG/test1/test3 > >> > +TEST_CG_SUB4=$TEST_ROOT_CG/test4 > >> > + > >> > +cgcreate -g misc:$TEST_CG_SUB1 > >> > >> > >> > >> >
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Thu Mar 28, 2024 at 5:54 AM EET, Haitao Huang wrote: > On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen > wrote: > > > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > >> The scripts rely on cgroup-tools package from libcgroup [1]. > >> > >> To run selftests for epc cgroup: > >> > >> sudo ./run_epc_cg_selftests.sh > >> > >> To watch misc cgroup 'current' changes during testing, run this in a > >> separate terminal: > >> > >> ./watch_misc_for_tests.sh current > >> > >> With different cgroups, the script starts one or multiple concurrent > >> SGX > >> selftests, each to run one unclobbered_vdso_oversubscribed test.Each > >> of such test tries to load an enclave of EPC size equal to the EPC > >> capacity available on the platform. The script checks results against > >> the expectation set for each cgroup and reports success or failure. > >> > >> The script creates 3 different cgroups at the beginning with > >> following > >> expectations: > >> > >> 1) SMALL - intentionally small enough to fail the test loading an > >> enclave of size equal to the capacity. > >> 2) LARGE - large enough to run up to 4 concurrent tests but fail some > >> if > >> more than 4 concurrent tests are run. The script starts 4 expecting > >> at > >> least one test to pass, and then starts 5 expecting at least one test > >> to fail. > >> 3) LARGER - limit is the same as the capacity, large enough to run > >> lots of > >> concurrent tests. The script starts 8 of them and expects all pass. > >> Then it reruns the same test with one process randomly killed and > >> usage checked to be zero after all process exit. > >> > >> The script also includes a test with low mem_cg limit and LARGE > >> sgx_epc > >> limit to verify that the RAM used for per-cgroup reclamation is > >> charged > >> to a proper mem_cg. > >> > >> [1] https://github.com/libcgroup/libcgroup/blob/main/README > >> > >> Signed-off-by: Haitao Huang > >> --- > >> V7: > >> - Added memcontrol test. > >> > >> V5: > >> - Added script with automatic results checking, remove the > >> interactive > >> script. > >> - The script can run independent from the series below. > >> --- > >> .../selftests/sgx/run_epc_cg_selftests.sh | 246 > >> ++ > >> .../selftests/sgx/watch_misc_for_tests.sh | 13 + > >> 2 files changed, 259 insertions(+) > >> create mode 100755 > >> tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> create mode 100755 > >> tools/testing/selftests/sgx/watch_misc_for_tests.sh > >> > >> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> new file mode 100755 > >> index ..e027bf39f005 > >> --- /dev/null > >> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > >> @@ -0,0 +1,246 @@ > >> +#!/bin/bash > > > > This is not portable and neither does hold in the wild. > > > > It does not even often hold as it is not uncommon to place bash > > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > > a path that is neither of those two. > > > > Should be #!/usr/bin/env bash > > > > That is POSIX compatible form. > > > > Sure > > > Just got around trying to test this in NUC7 so looking into this in > > more detail. > > Thanks. Could you please check if this version works for you? > > https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f > > > > > That said can you make the script work with just "#!/usr/bin/env sh" > > and make sure that it is busybox ash compatible? > > Yes. > > > > > I don't see any necessity to make this bash only and it adds to the > > compilation time of the image. Otherwise lot of this could be tested > > just with qemu+bzImage+busybox(inside initramfs). > > > > will still need cgroup-tools as you pointed out later. Compiling from its > upstream code OK? Can you explain why you need it? What is the thing you cannot do without it? BR, Jarkko
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Wed, 27 Mar 2024 07:55:34 -0500, Jarkko Sakkinen wrote: On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: The scripts rely on cgroup-tools package from libcgroup [1]. To run selftests for epc cgroup: sudo ./run_epc_cg_selftests.sh To watch misc cgroup 'current' changes during testing, run this in a separate terminal: ./watch_misc_for_tests.sh current With different cgroups, the script starts one or multiple concurrent SGX selftests, each to run one unclobbered_vdso_oversubscribed test.Each of such test tries to load an enclave of EPC size equal to the EPC capacity available on the platform. The script checks results against the expectation set for each cgroup and reports success or failure. The script creates 3 different cgroups at the beginning with following expectations: 1) SMALL - intentionally small enough to fail the test loading an enclave of size equal to the capacity. 2) LARGE - large enough to run up to 4 concurrent tests but fail some if more than 4 concurrent tests are run. The script starts 4 expecting at least one test to pass, and then starts 5 expecting at least one test to fail. 3) LARGER - limit is the same as the capacity, large enough to run lots of concurrent tests. The script starts 8 of them and expects all pass. Then it reruns the same test with one process randomly killed and usage checked to be zero after all process exit. The script also includes a test with low mem_cg limit and LARGE sgx_epc limit to verify that the RAM used for per-cgroup reclamation is charged to a proper mem_cg. [1] https://github.com/libcgroup/libcgroup/blob/main/README Signed-off-by: Haitao Huang --- V7: - Added memcontrol test. V5: - Added script with automatic results checking, remove the interactive script. - The script can run independent from the series below. --- .../selftests/sgx/run_epc_cg_selftests.sh | 246 ++ .../selftests/sgx/watch_misc_for_tests.sh | 13 + 2 files changed, 259 insertions(+) create mode 100755 tools/testing/selftests/sgx/run_epc_cg_selftests.sh create mode 100755 tools/testing/selftests/sgx/watch_misc_for_tests.sh diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh new file mode 100755 index ..e027bf39f005 --- /dev/null +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh @@ -0,0 +1,246 @@ +#!/bin/bash This is not portable and neither does hold in the wild. It does not even often hold as it is not uncommon to place bash to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has a path that is neither of those two. Should be #!/usr/bin/env bash That is POSIX compatible form. Sure Just got around trying to test this in NUC7 so looking into this in more detail. Thanks. Could you please check if this version works for you? https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f That said can you make the script work with just "#!/usr/bin/env sh" and make sure that it is busybox ash compatible? Yes. I don't see any necessity to make this bash only and it adds to the compilation time of the image. Otherwise lot of this could be tested just with qemu+bzImage+busybox(inside initramfs). will still need cgroup-tools as you pointed out later. Compiling from its upstream code OK? Now you are adding fully glibc shenanigans for the sake of syntax sugar. +# SPDX-License-Identifier: GPL-2.0 +# Copyright(c) 2023 Intel Corporation. + +TEST_ROOT_CG=selftest +cgcreate -g misc:$TEST_ROOT_CG How do you know that cgcreate exists? It is used a lot in the script with no check for the existence. Please fix e.g. with "command -v cgreate". +if [ $? -ne 0 ]; then +echo "# Please make sure cgroup-tools is installed, and misc cgroup is mounted." +exit 1 +fi And please do not do it this way. Also, please remove the advice for "cgroups-tool". This is not meant to be debian only. Better would be to e.g. point out the URL of the upstream project. And yeah the whole message should be based on "command -v", not like this. OK +TEST_CG_SUB1=$TEST_ROOT_CG/test1 +TEST_CG_SUB2=$TEST_ROOT_CG/test2 +# We will only set limit in test1 and run tests in test3 +TEST_CG_SUB3=$TEST_ROOT_CG/test1/test3 +TEST_CG_SUB4=$TEST_ROOT_CG/test4 + +cgcreate -g misc:$TEST_CG_SUB1 +cgcreate -g misc:$TEST_CG_SUB2 +cgcreate -g misc:$TEST_CG_SUB3 +cgcreate -g misc:$TEST_CG_SUB4 + +# Default to V2 +CG_MISC_ROOT=/sys/fs/cgroup +CG_MEM_ROOT=/sys/fs/cgroup +CG_V1=0 +if [ ! -d "/sys/fs/cgroup/misc" ]; then +echo "# cgroup V2 is in use." +else +echo "# cgroup V1 is in use." Is "#" prefix a standard for kselftest? I don't know this, thus asking. +CG_MISC_ROOT=/sys/fs/cgroup/misc +CG_MEM_ROOT=/sys/fs/cgroup/memory +CG_V1=1 Have you checked what is the indentation policy for bash scripts inside kernel tree. I don't know what it is. That's why I'm asking. Right. I looked around and found scripts
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Wed, 27 Mar 2024 19:57:26 -0500, Haitao Huang
wrote:
On Wed, 27 Mar 2024 11:56:35 -0500, Jarkko Sakkinen
wrote:
On Wed Mar 27, 2024 at 2:55 PM EET, Jarkko Sakkinen wrote:
On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote:
> The scripts rely on cgroup-tools package from libcgroup [1].
>
> To run selftests for epc cgroup:
>
> sudo ./run_epc_cg_selftests.sh
>
> To watch misc cgroup 'current' changes during testing, run this in a
> separate terminal:
...
> wait_and_detect_for_any() {
what is "any"?
Maybe for some key functions could have short documentation what they
are and for what test uses them. I cannot possibly remember all of this
just by hints such as "this waits for Any" ;-)
I don't think there is actual kernel guideline to engineer the script
to work with just ash but at least for me that would inevitably
increase my motivation to test this patch set more rather than less.
I also wonder is cgroup-tools dependency absolutely required or could
you just have a function that would interact with sysfs?
I should have checked email before hit the send button for v10 :-).
It'd be more complicated and less readable to do all the stuff without
the cgroup-tools, esp cgexec. I checked dependency, cgroup-tools only
depends on libc so I hope this would not cause too much inconvenience.
I saw bash was also used in cgroup test scripts so at least that's
consistent :-)
I can look into ash if that's required. Let me know.
Sorry I missed you earlier comments. I actually tried to make it
compatible with busybox on Ubuntu.
I'll address your other comments and update. But meanwhile could you
tryout this version in your env?
https://github.com/haitaohuang/linux/commit/3c424b841cf3cf66b085a424f4b537fbc3bbff6f
Thanks
Haitao
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Wed, 27 Mar 2024 11:56:35 -0500, Jarkko Sakkinen wrote: On Wed Mar 27, 2024 at 2:55 PM EET, Jarkko Sakkinen wrote: On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > The scripts rely on cgroup-tools package from libcgroup [1]. > > To run selftests for epc cgroup: > > sudo ./run_epc_cg_selftests.sh > > To watch misc cgroup 'current' changes during testing, run this in a > separate terminal: > > ./watch_misc_for_tests.sh current > > With different cgroups, the script starts one or multiple concurrent > SGX > selftests, each to run one unclobbered_vdso_oversubscribed test.> Each > of such test tries to load an enclave of EPC size equal to the EPC > capacity available on the platform. The script checks results against > the expectation set for each cgroup and reports success or failure. > > The script creates 3 different cgroups at the beginning with > following > expectations: > > 1) SMALL - intentionally small enough to fail the test loading an > enclave of size equal to the capacity. > 2) LARGE - large enough to run up to 4 concurrent tests but fail some > if > more than 4 concurrent tests are run. The script starts 4 expecting > at > least one test to pass, and then starts 5 expecting at least one test > to fail. > 3) LARGER - limit is the same as the capacity, large enough to run > lots of > concurrent tests. The script starts 8 of them and expects all pass. > Then it reruns the same test with one process randomly killed and > usage checked to be zero after all process exit. > > The script also includes a test with low mem_cg limit and LARGE > sgx_epc > limit to verify that the RAM used for per-cgroup reclamation is > charged > to a proper mem_cg. > > [1] https://github.com/libcgroup/libcgroup/blob/main/README > > Signed-off-by: Haitao Huang > --- > V7: > - Added memcontrol test. > > V5: > - Added script with automatic results checking, remove the > interactive > script. > - The script can run independent from the series below. > --- > .../selftests/sgx/run_epc_cg_selftests.sh | 246 > ++ > .../selftests/sgx/watch_misc_for_tests.sh | 13 + > 2 files changed, 259 insertions(+) > create mode 100755 > tools/testing/selftests/sgx/run_epc_cg_selftests.sh > create mode 100755 > tools/testing/selftests/sgx/watch_misc_for_tests.sh > > diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > new file mode 100755 > index ..e027bf39f005 > --- /dev/null > +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > @@ -0,0 +1,246 @@ > +#!/bin/bash This is not portable and neither does hold in the wild. It does not even often hold as it is not uncommon to place bash to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has a path that is neither of those two. Should be #!/usr/bin/env bash That is POSIX compatible form. Just got around trying to test this in NUC7 so looking into this in more detail. That said can you make the script work with just "#!/usr/bin/env sh" and make sure that it is busybox ash compatible? I don't see any necessity to make this bash only and it adds to the compilation time of the image. Otherwise lot of this could be tested just with qemu+bzImage+busybox(inside initramfs). Now you are adding fully glibc shenanigans for the sake of syntax sugar. > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright(c) 2023 Intel Corporation. > + > +TEST_ROOT_CG=selftest > +cgcreate -g misc:$TEST_ROOT_CG How do you know that cgcreate exists? It is used a lot in the script with no check for the existence. Please fix e.g. with "command -v cgreate". > +if [ $? -ne 0 ]; then > +echo "# Please make sure cgroup-tools is installed, and misc > cgroup is mounted." > +exit 1 > +fi And please do not do it this way. Also, please remove the advice for "cgroups-tool". This is not meant to be debian only. Better would be to e.g. point out the URL of the upstream project. And yeah the whole message should be based on "command -v", not like this. > +TEST_CG_SUB1=$TEST_ROOT_CG/test1 > +TEST_CG_SUB2=$TEST_ROOT_CG/test2 > +# We will only set limit in test1 and run tests in test3 > +TEST_CG_SUB3=$TEST_ROOT_CG/test1/test3 > +TEST_CG_SUB4=$TEST_ROOT_CG/test4 > + > +cgcreate -g misc:$TEST_CG_SUB1 > +cgcreate -g misc:$TEST_CG_SUB2 > +cgcreate -g misc:$TEST_CG_SUB3 > +cgcreate -g misc:$TEST_CG_SUB4 > + > +# Default to V2 > +CG_MISC_ROOT=/sys/fs/cgroup > +CG_MEM_ROOT=/sys/fs/cgroup > +CG_V1=0 > +if [ ! -d "/sys/fs/cgroup/misc" ]; then > +echo "# cgroup V2 is in use." > +else > +echo "# cgroup V1 is in use." Is "#" prefix a standard for kselftest? I don't know this, thus asking. > +CG_MISC_ROOT=/sys/fs/cgroup/misc > +CG_MEM_ROOT=/sys/fs/cgroup/memory > +CG_V1=1 Have you checked what is the indentation policy for bash scripts inside kernel tree. I don't know what it is. That's why I'm asking. > +fi > + > +CAPACITY=$(grep "sgx_epc" "$CG_MISC_ROOT/misc.capacity" | awk
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Wed Mar 27, 2024 at 2:55 PM EET, Jarkko Sakkinen wrote: > On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote: > > The scripts rely on cgroup-tools package from libcgroup [1]. > > > > To run selftests for epc cgroup: > > > > sudo ./run_epc_cg_selftests.sh > > > > To watch misc cgroup 'current' changes during testing, run this in a > > separate terminal: > > > > ./watch_misc_for_tests.sh current > > > > With different cgroups, the script starts one or multiple concurrent > > SGX > > selftests, each to run one unclobbered_vdso_oversubscribed test. > > Each > > of such test tries to load an enclave of EPC size equal to the EPC > > capacity available on the platform. The script checks results against > > the expectation set for each cgroup and reports success or failure. > > > > The script creates 3 different cgroups at the beginning with > > following > > expectations: > > > > 1) SMALL - intentionally small enough to fail the test loading an > > enclave of size equal to the capacity. > > 2) LARGE - large enough to run up to 4 concurrent tests but fail some > > if > > more than 4 concurrent tests are run. The script starts 4 expecting > > at > > least one test to pass, and then starts 5 expecting at least one test > > to fail. > > 3) LARGER - limit is the same as the capacity, large enough to run > > lots of > > concurrent tests. The script starts 8 of them and expects all pass. > > Then it reruns the same test with one process randomly killed and > > usage checked to be zero after all process exit. > > > > The script also includes a test with low mem_cg limit and LARGE > > sgx_epc > > limit to verify that the RAM used for per-cgroup reclamation is > > charged > > to a proper mem_cg. > > > > [1] https://github.com/libcgroup/libcgroup/blob/main/README > > > > Signed-off-by: Haitao Huang > > --- > > V7: > > - Added memcontrol test. > > > > V5: > > - Added script with automatic results checking, remove the > > interactive > > script. > > - The script can run independent from the series below. > > --- > > .../selftests/sgx/run_epc_cg_selftests.sh | 246 > > ++ > > .../selftests/sgx/watch_misc_for_tests.sh | 13 + > > 2 files changed, 259 insertions(+) > > create mode 100755 > > tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > create mode 100755 > > tools/testing/selftests/sgx/watch_misc_for_tests.sh > > > > diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > new file mode 100755 > > index ..e027bf39f005 > > --- /dev/null > > +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh > > @@ -0,0 +1,246 @@ > > +#!/bin/bash > > This is not portable and neither does hold in the wild. > > It does not even often hold as it is not uncommon to place bash > to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has > a path that is neither of those two. > > Should be #!/usr/bin/env bash > > That is POSIX compatible form. > > Just got around trying to test this in NUC7 so looking into this in > more detail. > > That said can you make the script work with just "#!/usr/bin/env sh" > and make sure that it is busybox ash compatible? > > I don't see any necessity to make this bash only and it adds to the > compilation time of the image. Otherwise lot of this could be tested > just with qemu+bzImage+busybox(inside initramfs). > > Now you are adding fully glibc shenanigans for the sake of syntax > sugar. > > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright(c) 2023 Intel Corporation. > > + > > +TEST_ROOT_CG=selftest > > +cgcreate -g misc:$TEST_ROOT_CG > > How do you know that cgcreate exists? It is used a lot in the script > with no check for the existence. Please fix e.g. with "command -v > cgreate". > > > +if [ $? -ne 0 ]; then > > + echo "# Please make sure cgroup-tools is installed, and misc > > cgroup is mounted." > > + exit 1 > > +fi > > And please do not do it this way. Also, please remove the advice for > "cgroups-tool". This is not meant to be debian only. Better would be > to e.g. point out the URL of the upstream project. > > And yeah the whole message should be based on "command -v", not like > this. > > > +TEST_CG_SUB1=$TEST_ROOT_CG/test1 > > +TEST_CG_SUB2=$TEST_ROOT_CG/test2 > > +# We will only set limit in test1 and run tests in test3 > > +TEST_CG_SUB3=$TEST_ROOT_CG/test1/test3 > > +TEST_CG_SUB4=$TEST_ROOT_CG/test4 > > + > > +cgcreate -g misc:$TEST_CG_SUB1 > > > > > +cgcreate -g misc:$TEST_CG_SUB2 > > +cgcreate -g misc:$TEST_CG_SUB3 > > +cgcreate -g misc:$TEST_CG_SUB4 > > + > > +# Default to V2 > > +CG_MISC_ROOT=/sys/fs/cgroup > > +CG_MEM_ROOT=/sys/fs/cgroup > > +CG_V1=0 > > +if [ ! -d "/sys/fs/cgroup/misc" ]; then > > + echo "# cgroup V2 is in use." > > +else > > + echo "# cgroup V1 is in use." > > Is "#" prefix a standard for kselftest? I don't know this, thus asking. > > > + CG_MISC_ROOT=/sys/fs/cgroup/misc > > + CG_MEM_ROOT=/sys/fs/cgroup/memory
Re: [PATCH v9 15/15] selftests/sgx: Add scripts for EPC cgroup testing
On Mon, 2024-02-05 at 13:06 -0800, Haitao Huang wrote:
> The scripts rely on cgroup-tools package from libcgroup [1].
>
> To run selftests for epc cgroup:
>
> sudo ./run_epc_cg_selftests.sh
>
> To watch misc cgroup 'current' changes during testing, run this in a
> separate terminal:
>
> ./watch_misc_for_tests.sh current
>
> With different cgroups, the script starts one or multiple concurrent
> SGX
> selftests, each to run one unclobbered_vdso_oversubscribed test.
> Each
> of such test tries to load an enclave of EPC size equal to the EPC
> capacity available on the platform. The script checks results against
> the expectation set for each cgroup and reports success or failure.
>
> The script creates 3 different cgroups at the beginning with
> following
> expectations:
>
> 1) SMALL - intentionally small enough to fail the test loading an
> enclave of size equal to the capacity.
> 2) LARGE - large enough to run up to 4 concurrent tests but fail some
> if
> more than 4 concurrent tests are run. The script starts 4 expecting
> at
> least one test to pass, and then starts 5 expecting at least one test
> to fail.
> 3) LARGER - limit is the same as the capacity, large enough to run
> lots of
> concurrent tests. The script starts 8 of them and expects all pass.
> Then it reruns the same test with one process randomly killed and
> usage checked to be zero after all process exit.
>
> The script also includes a test with low mem_cg limit and LARGE
> sgx_epc
> limit to verify that the RAM used for per-cgroup reclamation is
> charged
> to a proper mem_cg.
>
> [1] https://github.com/libcgroup/libcgroup/blob/main/README
>
> Signed-off-by: Haitao Huang
> ---
> V7:
> - Added memcontrol test.
>
> V5:
> - Added script with automatic results checking, remove the
> interactive
> script.
> - The script can run independent from the series below.
> ---
> .../selftests/sgx/run_epc_cg_selftests.sh | 246
> ++
> .../selftests/sgx/watch_misc_for_tests.sh | 13 +
> 2 files changed, 259 insertions(+)
> create mode 100755
> tools/testing/selftests/sgx/run_epc_cg_selftests.sh
> create mode 100755
> tools/testing/selftests/sgx/watch_misc_for_tests.sh
>
> diff --git a/tools/testing/selftests/sgx/run_epc_cg_selftests.sh
> b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh
> new file mode 100755
> index ..e027bf39f005
> --- /dev/null
> +++ b/tools/testing/selftests/sgx/run_epc_cg_selftests.sh
> @@ -0,0 +1,246 @@
> +#!/bin/bash
This is not portable and neither does hold in the wild.
It does not even often hold as it is not uncommon to place bash
to the path /usr/bin/bash. If I recall correctly, e.g. NixOS has
a path that is neither of those two.
Should be #!/usr/bin/env bash
That is POSIX compatible form.
Just got around trying to test this in NUC7 so looking into this in
more detail.
That said can you make the script work with just "#!/usr/bin/env sh"
and make sure that it is busybox ash compatible?
I don't see any necessity to make this bash only and it adds to the
compilation time of the image. Otherwise lot of this could be tested
just with qemu+bzImage+busybox(inside initramfs).
Now you are adding fully glibc shenanigans for the sake of syntax
sugar.
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright(c) 2023 Intel Corporation.
> +
> +TEST_ROOT_CG=selftest
> +cgcreate -g misc:$TEST_ROOT_CG
How do you know that cgcreate exists? It is used a lot in the script
with no check for the existence. Please fix e.g. with "command -v
cgreate".
> +if [ $? -ne 0 ]; then
> + echo "# Please make sure cgroup-tools is installed, and misc
> cgroup is mounted."
> + exit 1
> +fi
And please do not do it this way. Also, please remove the advice for
"cgroups-tool". This is not meant to be debian only. Better would be
to e.g. point out the URL of the upstream project.
And yeah the whole message should be based on "command -v", not like
this.
> +TEST_CG_SUB1=$TEST_ROOT_CG/test1
> +TEST_CG_SUB2=$TEST_ROOT_CG/test2
> +# We will only set limit in test1 and run tests in test3
> +TEST_CG_SUB3=$TEST_ROOT_CG/test1/test3
> +TEST_CG_SUB4=$TEST_ROOT_CG/test4
> +
> +cgcreate -g misc:$TEST_CG_SUB1
> +cgcreate -g misc:$TEST_CG_SUB2
> +cgcreate -g misc:$TEST_CG_SUB3
> +cgcreate -g misc:$TEST_CG_SUB4
> +
> +# Default to V2
> +CG_MISC_ROOT=/sys/fs/cgroup
> +CG_MEM_ROOT=/sys/fs/cgroup
> +CG_V1=0
> +if [ ! -d "/sys/fs/cgroup/misc" ]; then
> + echo "# cgroup V2 is in use."
> +else
> + echo "# cgroup V1 is in use."
Is "#" prefix a standard for kselftest? I don't know this, thus asking.
> + CG_MISC_ROOT=/sys/fs/cgroup/misc
> + CG_MEM_ROOT=/sys/fs/cgroup/memory
> + CG_V1=1
Have you checked what is the indentation policy for bash scripts inside
kernel tree. I don't know what it is. That's why I'm asking.
> +fi
> +
> +CAPACITY=$(grep "sgx_epc" "$CG_MISC_ROOT/misc.capacity" | awk
> '{print $2}')
> +# This is below number of VA pages needed for enclave of capacity
> size. So
> +# should
