rt-tests: Drop use_current_cpuset() check
PATCH 1/1 rt-tests: Drop use_current_cpuset() check
CPU list should allow to be any list rather than only the cores that are allowed to schedule for the current process.
Before this patch, cyclictest will fail with below condition:
$ taskset -pc $$ pid 2316's current affinity list: 0,2,4,6,8 $ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4 WARN: Couldn't setaffinity in main thread: Invalid argument
After this patch, it'll be allowed to run.
--- src/lib/rt-numa.c | 27 --------------------------- 1 file changed, 27 deletions(-)
src/lib/rt-numa.c +0 -27
@@ -93,32 +93,6 @@ int cpu_for_thread_ua(int thread_num, int max_cpus)return 0 ; } -/* -* After this function is called, affinity_mask is the intersection of -* the user supplied affinity mask and the affinity mask from the run -* time environment -*/ -static void use_current_cpuset (int max_cpus,struct bitmask *cpumask) -{ -struct bitmask *curmask; -int i; - - curmask =numa_allocate_cpumask (); -numa_sched_getaffinity (getpid (), curmask); - -/* -* Clear bits that are not set in both the cpuset from the -* environment, and in the user specified affinity. -*/ -for (i =0 ; i < max_cpus; i++) { -if ((!numa_bitmask_isbitset (cpumask, i)) || - (!numa_bitmask_isbitset (curmask, i))) -numa_bitmask_clearbit (cpumask, i); - } - -numa_bitmask_free (curmask); -} -int parse_cpumask (char *str,int max_cpus,struct bitmask **cpumask) {struct bitmask *mask;
@@ -133,7 +107,6 @@ int parse_cpumask(char *str, int max_cpus, struct bitmask **cpumask)return 0 ; } -use_current_cpuset (max_cpus, mask); *cpumask = mask;return 0 ;
Signature · 1 line
2.26.2
Hi Peter,
Quoted 2 lines · Peter Xu
CPU list should allow to be any list rather than only the cores that are allowed to schedule for the current process.
See commit aaa57168dfd3 ("rt-tests: cyclictest: Only run on runtime affinity and user supplied affinity")
Quoted 8 lines
Before this patch, cyclictest will fail with below condition:
$ taskset -pc $$ pid 2316's current affinity list: 0,2,4,6,8 $ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4 WARN: Couldn't setaffinity in main thread: Invalid argumentAfter this patch, it'll be allowed to run.
As John writes in the commit above message I think cyclictest should honor to the runtime settings. The warning above could be extended and telling you what's the problem is.
Your commit message should contain the argument why it's better not to check the environment settings. So the question is what is the least surprise for the user?
Thanks,
Daniel
Quoted 1 line · Daniel Wagner
Hi Peter,
Daniel,
Quoted 7 lines · Peter Xu
Quoted 2 lines
CPU list should allow to be any list rather than only the cores that are allowed to schedule for the current process.
See commit
aaa57168dfd3("rt-tests: cyclictest: Only run on runtime affinity and user supplied affinity")
(Thanks for the commit ID)
Quoted 10 lines
Quoted 9 lines
Before this patch, cyclictest will fail with below condition:
$ taskset -pc $$ pid 2316's current affinity list: 0,2,4,6,8 $ sudo cyclictest -m -N -p 1 -a 1,3,5,7 -t 4 WARN: Couldn't setaffinity in main thread: Invalid argumentAfter this patch, it'll be allowed to run.
Quoted 7 lines
As John writes in the commit above message I think cyclictest should honor to the runtime settings. The warning above could be extended and telling you what's the problem is.
Your commit message should contain the argument why it's better not to check the environment settings. So the question is what is the least surprise for the user?
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
Currently if the user passes the affinity to cyclictest, threads are run there even if they should be excluded according to the affinity of the runtime environment.
So I'm not sure I understand the word "runtime environment".
If it's defined as "the set of cores that this process is allowed to run", I don't understand why it's not allowed to schedule things outside of this set of cores, especially if sched_setaffinity() syscall would succeed. IOW, I'm afraid we got the idea slightly mixed up on "cores allowed to schedule this process" with "cores allowed to be set as schedule target by this process", and IMHO we should simply rely on sched_setaffinity() syscall to decide whether the affinity setting is legal or not as a single checkpoint.
John and I had some discussion offlist about this last time on oslat, it should be the same thing here I think. But I'd like John to help confirm too since I could be missing something.
Thanks,
Signature · 1 line
Peter Xu
Quoted 2 lines · Peter Xu
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
IIRC, the current behavior allows the process to be placed into a cgroup with a subset of CPUs and you just can do 'cyclictest -a -t'. Process should not ignore external configuration. That's my whole point here.
Quoted 4 lines
So I'm not sure I understand the word "runtime environment".
If it's defined as "the set of cores that this process is allowed to run",
I am trying to say is, the tests should not assume they have the full control of the placement as this is not what I would expect. If I as 'admin' limits the sched mask then cyclictest should not overwrite it.
If you insist removing this code, please add a section in the documentation explaining why the tools ignore it.
Quoted 1 line
John and I had some discussion offlist about this last time on oslat,
As I said, I think this behavior is wrong. Anyway, I stopped caring.
Hi, Daniel,
Quoted 7 lines · Daniel Wagner
Quoted 2 lines · Peter Xu
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
IIRC, the current behavior allows the process to be placed into a cgroup with a subset of CPUs and you just can do 'cyclictest -a -t'. Process should not ignore external configuration. That's my whole point here.
In that case again I think a sane solution is not to check the cpu list in every single tool we use, because even if we do that for all tools in rt-teets repo, we can't guarantee to have this check for the rest tools to not ignore this restriction.
A simple example is: what if the user specified "taskset -c $CPU cyclictest -a $CPU -t 1 ..." where $CPU is not in the allowed list of current bash? As long as the taskset would work the so-called "environment" will be changed before even loading cyclictest.
If you see that's the point I said we should fail at the same check point of sched_setaffinity() rather than checking it explicitly in the tool, because if we want a real-world restriction that's the only place I think it's possible..
But I'm not a cgroup/container guy, please correct me if I understood.
Signature · 1 line
Peter Xu
Quoted 21 lines · Peter Xu
Hi, Daniel,
Quoted 7 lines · Daniel Wagner
Quoted 2 lines · Peter Xu
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
IIRC, the current behavior allows the process to be placed into a cgroup with a subset of CPUs and you just can do 'cyclictest -a -t'. Process should not ignore external configuration. That's my whole point here.
In that case again I think a sane solution is not to check the cpu list in every single tool we use, because even if we do that for all tools in rt-teets repo, we can't guarantee to have this check for the rest tools to not ignore this restriction.
A simple example is: what if the user specified "taskset -c $CPU cyclictest -a $CPU -t 1 ..." where $CPU is not in the allowed list of current bash? As long as the taskset would work the so-called "environment" will be changed before even loading cyclictest.
Quoted 10 lines
If you see that's the point I said we should fail at the same check point of
sched_setaffinity()rather than checking it explicitly in the tool, because if we want a real-world restriction that's the only place I think it's possible..But I'm not a cgroup/container guy, please correct me if I understood.
Signature · 1 line
Peter Xu
When cyclictest and friends were originally written, we had this view point that we "owned" the whole machine, and didn't have any restrictions on where to schedule. As machines grew in size, and we added numa awareness, and cgroups became more prominent we added this code that tried to schedule according to the ill-defined environment that we found ourselves in.
As Peter points out we may have restricted ourselves more than is necessary, and can rely a bit more on the operating system to restrict us. On the otherhand using taskset is an easy workaround if the current code is to restrictive.
Because we can use taskset and things are working well otherwise I don't see this as super urgent, but I am willing to revisit this code and make it less restrictive if that makes sense.
I also am not a cgroup / container person, and would like to play around with this a bit more before we make some decisions on which direction to go in.
Does that make sense to everyone?
John
Quoted 52 lines · John Kacur
Quoted 21 lines · Peter Xu
Hi, Daniel,
Quoted 7 lines · Daniel Wagner
Quoted 2 lines · Peter Xu
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
IIRC, the current behavior allows the process to be placed into a cgroup with a subset of CPUs and you just can do 'cyclictest -a -t'. Process should not ignore external configuration. That's my whole point here.
In that case again I think a sane solution is not to check the cpu list in every single tool we use, because even if we do that for all tools in rt-teets repo, we can't guarantee to have this check for the rest tools to not ignore this restriction.
A simple example is: what if the user specified "taskset -c $CPU cyclictest -a $CPU -t 1 ..." where $CPU is not in the allowed list of current bash? As long as the taskset would work the so-called "environment" will be changed before even loading cyclictest.
Quoted 10 lines
If you see that's the point I said we should fail at the same check point of
sched_setaffinity()rather than checking it explicitly in the tool, because if we want a real-world restriction that's the only place I think it's possible..But I'm not a cgroup/container guy, please correct me if I understood.
Signature · 1 line
Peter XuWhen cyclictest and friends were originally written, we had this view point that we "owned" the whole machine, and didn't have any restrictions on where to schedule. As machines grew in size, and we added numa awareness, and cgroups became more prominent we added this code that tried to schedule according to the ill-defined environment that we found ourselves in.
As Peter points out we may have restricted ourselves more than is necessary, and can rely a bit more on the operating system to restrict us. On the otherhand using taskset is an easy workaround if the current code is to restrictive.
Because we can use taskset and things are working well otherwise I don't see this as super urgent, but I am willing to revisit this code and make it less restrictive if that makes sense.
Quoted 5 lines
I also am not a cgroup / container person, and would like to play around with this a bit more before we make some decisions on which direction to go in.
Does that make sense to everyone?
Sure thing on my side. No bug reported so far this time, so I'll wait at least until then :) I just don't know why it's not hit just like oslat since I don't see a difference. When I fixed the oslat thing, I thought cyclictest didn't have such issue for some reason so I didn't consider to touch it at all. But when yesterday I rerun some tests I see this issue on rhel8, hence this patch.
Thanks,
Signature · 1 line
Peter Xu
Quoted 59 lines · Peter Xu
Quoted 52 lines · John Kacur
Quoted 21 lines · Peter Xu
Hi, Daniel,
Quoted 7 lines · Daniel Wagner
Quoted 2 lines · Peter Xu
I think what I'm missing is why we had such a restriction. Quotting from the commit ID:
IIRC, the current behavior allows the process to be placed into a cgroup with a subset of CPUs and you just can do 'cyclictest -a -t'. Process should not ignore external configuration. That's my whole point here.
In that case again I think a sane solution is not to check the cpu list in every single tool we use, because even if we do that for all tools in rt-teets repo, we can't guarantee to have this check for the rest tools to not ignore this restriction.
A simple example is: what if the user specified "taskset -c $CPU cyclictest -a $CPU -t 1 ..." where $CPU is not in the allowed list of current bash? As long as the taskset would work the so-called "environment" will be changed before even loading cyclictest.
Quoted 10 lines
If you see that's the point I said we should fail at the same check point of
sched_setaffinity()rather than checking it explicitly in the tool, because if we want a real-world restriction that's the only place I think it's possible..But I'm not a cgroup/container guy, please correct me if I understood.
Signature · 1 line
Peter XuWhen cyclictest and friends were originally written, we had this view point that we "owned" the whole machine, and didn't have any restrictions on where to schedule. As machines grew in size, and we added numa awareness, and cgroups became more prominent we added this code that tried to schedule according to the ill-defined environment that we found ourselves in.
As Peter points out we may have restricted ourselves more than is necessary, and can rely a bit more on the operating system to restrict us. On the otherhand using taskset is an easy workaround if the current code is to restrictive.
Because we can use taskset and things are working well otherwise I don't see this as super urgent, but I am willing to revisit this code and make it less restrictive if that makes sense.
Quoted 5 lines
I also am not a cgroup / container person, and would like to play around with this a bit more before we make some decisions on which direction to go in.
Does that make sense to everyone?
Quoted 12 lines
Sure thing on my side. No bug reported so far this time, so I'll wait at least until then :) I just don't know why it's not hit just like oslat since I don't see a difference. When I fixed the oslat thing, I thought cyclictest didn't have such issue for some reason so I didn't consider to touch it at all. But when yesterday I rerun some tests I see this issue on rhel8, hence this patch.
Thanks,
Signature · 1 line
Peter Xu
Thanks Peter - I appreciate it!
John
Quoted 7 lines · John Kacur
Quoted 7 lines
Quoted 1 line
Does that make sense to everyone?
Sure thing on my side. No bug reported so far this time, so I'll wait at least until then :) I just don't know why it's not hit just like oslat since I don't see a difference. When I fixed the oslat thing, I thought cyclictest didn't have such issue for some reason so I didn't consider to touch it at all. But when yesterday I rerun some tests I see this issue on rhel8, hence this patch.
Don't let me stop you ripping out the code. I am not agreeing with the reasoning why it should go but it's a detail I don't care.
Quoted 5 lines · John Kacur
I also am not a cgroup / container person, and would like to play around with this a bit more before we make some decisions on which direction to go in.
Pardon my ignorance, but (scheduling wise) what does an RT task inside a container/cgroup mean?
AFAIK, the RT task will still command the CPU as it pleases, no matter the cgroup's cpu.shares configuration.
Thanks,
--
Ahmed S. Darwish
Quoted 5 lines · Ahmed S. Darwish
Pardon my ignorance, but (scheduling wise) what does an RT task inside a container/cgroup mean?
AFAIK, the RT task will still command the CPU as it pleases, no matter the cgroup's cpu.shares configuration.
It's not about scheduling it's about affinity only.