oslat fixes
cover oslat fixes
A couple of fixes for the bugs I introduced recently, reported by Peter.
Daniel Wagner (2):
oslat: Print version string
oslat: Use cpuset size as upper bound
src/oslat/oslat.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
Signature · 1 line
2.30.0
PATCH 1/2 oslat: Print version string
During the streamlining of the command line options we missed to hook up the command line option '--version' with usage().
--- src/oslat/oslat.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
src/oslat/oslat.c +1 -7
@@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])*/ g.single_preheat_thread = true;break ; -case 'v' : -/* -* Because we always dump the version even before parsing options, -* what we need to do is to quit.. -*/ -exit (0 ); -break ;case 'z' : g.output_omit_zero_buckets =1 ;break ; +case 'v' :case 'h' :usage (0 );break ;
Signature · 1 line
2.30.0
Re: [PATCH rt-tests 1/2] oslat: Print version string
Hi, Daniel,
Quoted 34 lines · Daniel Wagner
During the streamlining of the command line options we missed to hook up the command line option '--version' with
usage().Fixes e411219d27b1 ("oslat: Streamline usage output and man page")Reported-by Peter Xu <[email protected]>Signed-off-by Daniel Wagner <[email protected]>--- src/oslat/oslat.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)src/oslat/oslat.c +1
-7@@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])*/ g.single_preheat_thread = true;break ; -case 'v' : -/* -* Because we always dump the version even before parsing options, -* what we need to do is to quit.. -*/ -exit (0 ); -break ;case 'z' : g.output_omit_zero_buckets =1 ;break ; +case 'v' :case 'h' :usage (0 );break ;Signature · 1 line
2.30.0
Thanks for your quick response. I've commented here:
https://lore.kernel.org/linux-rt-users/20210210170015.GL103365@xz-x1/
Signature · 1 line
Peter Xu
Re: [PATCH rt-tests 1/2] oslat: Print version string
In reply to Daniel WagnerQuoted 36 lines · Daniel Wagner
During the streamlining of the command line options we missed to hook up the command line option '--version' with
usage().Fixes e411219d27b1 ("oslat: Streamline usage output and man page")Reported-by Peter Xu <[email protected]>Signed-off-by Daniel Wagner <[email protected]>--- src/oslat/oslat.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)src/oslat/oslat.c +1
-7@@ -655,16 +655,10 @@ static void parse_options(int argc, char *argv[])*/ g.single_preheat_thread = true;break ; -case 'v' : -/* -* Because we always dump the version even before parsing options, -* what we need to do is to quit.. -*/ -exit (0 ); -break ;case 'z' : g.output_omit_zero_buckets =1 ;break ; +case 'v' :case 'h' :usage (0 );break ;Signature · 1 line
2.30.0
It shouldn't be a fall through, we don't want to print the usage.
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 1 line · John Kacur
It shouldn't be a fall through, we don't want to print the usage.
Ok. In this case I suggest we introduce for all tolls the long option '--version' and print only the version string.
As I have already added the long parsing option in my series 'Generate machine-readable output'. This was a lot of tedious work, so I would like to avoid to do it split out for the '--version' option. Thus, could you review the series and I could easily fix this problem.
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 10 lines · Daniel Wagner
Quoted 1 line · John Kacur
It shouldn't be a fall through, we don't want to print the usage.
Ok. In this case I suggest we introduce for all tolls the long option '--version' and print only the version string.
As I have already added the long parsing option in my series 'Generate machine-readable output'. This was a lot of tedious work, so I would like to avoid to do it split out for the '--version' option. Thus, could you review the series and I could easily fix this problem.
It's great idea to have JSON format for all the tools. However I just don't see why "-v"/"--version" is related to your machine readable output work. Could you exlaborate? Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 3 lines
It's great idea to have JSON format for all the tools. However I just don't see why "-v"/"--version" is related to your machine readable output work. Could you exlaborate? Thanks,
I've added long options support to many rt-tests in the mentioned series. I would like to base this fix on top of this work.
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 6 lines · Daniel Wagner
Quoted 3 lines
It's great idea to have JSON format for all the tools. However I just don't see why "-v"/"--version" is related to your machine readable output work. Could you exlaborate? Thanks,
I've added long options support to many rt-tests in the mentioned series. I would like to base this fix on top of this work.
My opinion probably don't even matter here... but I tend to agree with John that the fix and your work is separated. If there's must a rebase, it would be better to rebase the big series upon the fix rather than the other way round, simply because a fix is normally more important than a new feature (if not talking about this "-v" thing, the other --cpu-list issue should be important).
Appreciate all the refactoring works on oslat happened and ongoing, it would just be even nicer if the touched code path can be smoked so we don't miss anything important.
Thanks again.
Signature · 1 line
Peter Xu
Re: [PATCH rt-tests 1/2] oslat: Print version string
In reply to Daniel WagnerQuoted 11 lines · Daniel Wagner
Quoted 1 line · John Kacur
It shouldn't be a fall through, we don't want to print the usage.
Ok. In this case I suggest we introduce for all tolls the long option '--version' and print only the version string.
As I have already added the long parsing option in my series 'Generate machine-readable output'. This was a lot of tedious work, so I would like to avoid to do it split out for the '--version' option. Thus, could you review the series and I could easily fix this problem.
Are you really telling me you won't fix a problem you introduced unless I first take all the other code you would like to get in rt-tests?
:)
John
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 2 lines · John Kacur
Are you really telling me you won't fix a problem you introduced unless I first take all the other code you would like to get in rt-tests?
I've already done the long option support in the JSON series. It would make my life way simpler if those bits would be in the tree.
I think, the simplest fix for now is to temporarily a local fix for oslat.
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 10 lines · Daniel Wagner
Quoted 2 lines · John Kacur
Are you really telling me you won't fix a problem you introduced unless I first take all the other code you would like to get in rt-tests?
I've already done the long option support in the JSON series. It would make my life way simpler if those bits would be in the tree.
I think, the simplest fix for now is to temporarily a local fix for oslat.
Just add a fix onto for the oslat problem on the end of your series I'll be done the review soon.
If we need to do any backporting then we'll do that.
Cheers, Thanks
John
Re: [PATCH rt-tests 1/2] oslat: Print version string
Quoted 15 lines · John Kacur
Quoted 10 lines · Daniel Wagner
Quoted 2 lines · John Kacur
Are you really telling me you won't fix a problem you introduced unless I first take all the other code you would like to get in rt-tests?
I've already done the long option support in the JSON series. It would make my life way simpler if those bits would be in the tree.
I think, the simplest fix for now is to temporarily a local fix for oslat.
Quoted 9 lines
Just add a fix onto for the oslat problem on the end of your series I'll be done the review soon.
If we need to do any backporting then we'll do that.
Cheers, Thanks
John
I rewound your patches in git, and applied the two oslat fixes and reapplied your patches with a git interactive rebase, and then I pushed them for others to try before I'm ready to push them upstream
Anyone who wants to try the new json stuff it's in a branch called
unstable/devel/rt-numa-json
The numa changes need more testing before integrate them, but this should make it easier for others to try it out.
John
PATCH 2/2 oslat: Use cpuset size as upper bound
In reply to Daniel WagnerTo assign the threads to the correct CPU we need to use the cpuset size as upper bound for the loop and not the number of threads.
--- src/oslat/oslat.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
src/oslat/oslat.c +5 -2
@@ -742,9 +742,12 @@ int main(int argc, char *argv[]) n_cores =numa_bitmask_weight (cpu_set);TEST (threads =calloc (1 , n_cores *sizeof (threads[0 ]))); -for (i =0 ; i < n_cores; ++i) -if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) +for (i =0 ; n_cores && i < cpu_set->size; i++) { +if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) { threads[g.n_threads_total++].core_i = i; + n_cores--; + } + }if (numa_bitmask_isbitset (cpu_set,0 ) && g.rtprio)printf ("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n" );
Signature · 1 line
2.30.0
Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound
Quoted 5 lines · Daniel Wagner
To assign the threads to the correct CPU we need to use the cpuset size as upper bound for the loop and not the number of threads.
Fixes 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")Reported-by Peter Xu <[email protected]>
Please instead add:
Pradipta, would you verify and see whether this patch fixes your issue?
Thanks,
Signature · 1 line
Peter Xu
Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound
In reply to Daniel WagnerQuoted 33 lines · Daniel Wagner
To assign the threads to the correct CPU we need to use the cpuset size as upper bound for the loop and not the number of threads.
Fixes 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")Reported-by Peter Xu <[email protected]>Signed-off-by Daniel Wagner <[email protected]>--- src/oslat/oslat.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)src/oslat/oslat.c +5
-2@@ -742,9 +742,12 @@ int main(int argc, char *argv[]) n_cores =numa_bitmask_weight (cpu_set);TEST (threads =calloc (1 , n_cores *sizeof (threads[0 ]))); -for (i =0 ; i < n_cores; ++i) -if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) +for (i =0 ; n_cores && i < cpu_set->size; i++) { +if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) { threads[g.n_threads_total++].core_i = i; + n_cores--; + } + }if (numa_bitmask_isbitset (cpu_set,0 ) && g.rtprio)printf ("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n" );Signature · 1 line
2.30.0
This looks okay, but we are waiting for Pradipta's testing to confirm before I push it.
Thanks
John
Re: [PATCH rt-tests 2/2] oslat: Use cpuset size as upper bound
Quoted 31 lines · John Kacur
Quoted 27 lines · Daniel Wagner
To assign the threads to the correct CPU we need to use the cpuset size as upper bound for the loop and not the number of threads.
Fixes 85b0763dacd9 ("oslat: Use parse_cpumask() from rt-numa.h")Reported-by Peter Xu <[email protected]>Signed-off-by Daniel Wagner <[email protected]>--- src/oslat/oslat.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)src/oslat/oslat.c +5
-2@@ -742,9 +742,12 @@ int main(int argc, char *argv[]) n_cores =numa_bitmask_weight (cpu_set);TEST (threads =calloc (1 , n_cores *sizeof (threads[0 ]))); -for (i =0 ; i < n_cores; ++i) -if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) +for (i =0 ; n_cores && i < cpu_set->size; i++) { +if (numa_bitmask_isbitset (cpu_set, i) &&move_to_core (i) ==0 ) { threads[g.n_threads_total++].core_i = i; + n_cores--; + } + }
Quoted 9 lines
Quoted 6 lines
src/oslat/oslat.c +0
-0if (numa_bitmask_isbitset (cpu_set,0 ) && g.rtprio)printf ("WARNING: Running SCHED_FIFO workload on CPU 0 may hang the thread\n" );Signature · 1 line
2.30.0This looks okay, but we are waiting for Pradipta's testing to confirm before I push it.
After the reporter's confirmation there seems to have two issues. Daniel's patch should be a valid fix of at least one of the problem. The other problem is still under investigation and I think it's not affecting the upstream branch. So I think we don't need to wait for that anymore:
Thanks,
Signature · 1 line
Peter Xu