xl: fix broken xl vcpu-list output
PATCH 1/1 xl: fix broken xl vcpu-list output
Hi,
# xl vcpu-list
hangs on my big box. The issue is an endless loop, where the algorithm
for printing the CPU affinity in a condensed way looks for a set bit in
a zero-byte:
for (i = 0; !(pcpumap & 1); ++i, pcpumap >>= 1)
Looking at the code I found that it is entirely broken if more than 8
CPUs are used. Beside that endless loop issue the output is totally
bogus except for the "any CPU" case, which is handled explicitly earlier.
I tried to fix it, but the whole approach does not work if the outer
loops actually iterates (executing more than once).
I could not copy the Linux version of that algorithm due to licensing
incompatibilities and the Python version is not easily converted to C,
so I coded my own version from scratch. It is a bit verbose since it
iterates over bits instead of bytes, but more cleaner and survived some
unit-testing. I didn't spend much time in optimizing it, though.
I put it in a separate function as I plan to use it later for printing
cpupool affinity in a similar way (a post 4.1.0 patch living in one of my branches).If you have a better implementation available, I can push it through my automated unit test easily.
Please review and apply to Xen 4.1.0-rc.
Regards,
Andre.
Signature · 2 lines
Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany
commit 3a002468a2ff1575d32a09353e976eb0312bda77
Author: Andre Przywara <[email protected]> Date: Fri Feb 4 14:57:05 2011 +0100
xl: fix broken xl vcpu-list output with more than 8 CPUs
where the algorithm for printing the CPU affinity in a condensed way looks for a set bit in a zero-byte:
for (i = 0; !(pcpumap & 1); ++i, pcpumap >>= 1)
Looking at the code I found that it is entirely broken if more than 8 CPUs are used. Beside that endless loop issue the output is totally bogus except for the "any CPU" case, which is handled explicitly earlier. I tried to fix it, but the whole approach does not work if the outer loops actually iterates (executing more than once).
This fix reimplements the whole algorithm in a clean (though not much optimized way). It survived some unit-testing.
tools/libxl/xl_cmdimpl.c +52 -44
@@ -3372,13 +3372,60 @@ int main_button_press(int argc, char **argv)return 0 ; } +static void print_bitmap (uint8_t *map,int maplen,FILE *stream) +{ +int i; +uint8_t pmap, bitmask; +int firstset, state =0 ; + +for (i =0 ; i < maplen; i++) { +if (i %8 ==0 ) { + pmap = *map++; + bitmask =1 ; + }else bitmask <<=1 ; + +switch (state) { +case 0 : +case 2 : +if ((pmap & bitmask) !=0 ) { + firstset = i; + state++; + } +continue ; +case 1 : +case 3 : +if ((pmap & bitmask) ==0 ) { +fprintf (stream,"%s%d" , state >1 ?"," :"" , firstset); +if (i -1 > firstset) +fprintf (stream,"-%d" , i -1 ); + state =2 ; + } +continue ; + } + } +switch (state) { +case 0 : +fprintf (stream,"none" ); +break ; +case 2 : +break ; +case 1 : +if (firstset ==0 ) { +fprintf (stream,"any cpu" ); +break ; + } +case 3 : +fprintf (stream,"%s%d" , state >1 ?"," :"" , firstset); +if (i -1 > firstset) +fprintf (stream,"-%d" , i -1 ); +break ; + } +} +static void print_vcpuinfo (uint32_t tdomid,const libxl_vcpuinfo *vcpuinfo,uint32_t nr_cpus) { -int i, l; -uint8_t *cpumap; -uint8_t pcpumap;char *domname;/* NAME ID VCPU */
@@ -3398,47 +3445,8 @@ static void print_vcpuinfo(uint32_t tdomid,/* TIM */ printf ("%9.1f " , ((float )vcpuinfo->vcpu_time /1e9 ));/* CPU AFFINITY */ - pcpumap = nr_cpus >8 ? (uint8_t )-1 : ((1 << nr_cpus) -1 ); -for (cpumap = vcpuinfo->cpumap.map; nr_cpus; ++cpumap) { -if (*cpumap < pcpumap) { -break ; - } -if (nr_cpus >8 ) { - pcpumap =-1 ; - nr_cpus -=8 ; - }else { - pcpumap = ((1 << nr_cpus) -1 ); - nr_cpus =0 ; - } - } -if (!nr_cpus) { -printf ("any cpu\n" ); - }else { -for (cpumap = vcpuinfo->cpumap.map; nr_cpus; ++cpumap) { - pcpumap = *cpumap; -for (i =0 ; !(pcpumap &1 ); ++i, pcpumap >>=1 ) - ; -printf ("%u" , i); -for (l = i, pcpumap = (pcpumap >>1 ); (pcpumap &1 ); ++i, pcpumap >>=1 ) - ; -if (l < i) { -printf ("-%u" , i); - } -for (++i; pcpumap; ++i, pcpumap >>=1 ) { -if (pcpumap &1 ) { -printf (",%u" , i); -for (l = i, pcpumap = (pcpumap >>1 ); (pcpumap &1 ); ++i, pcpumap >>=1 ) - ; -if (l < i) { -printf ("-%u" , i); - } - ++i; - } - } -printf ("\n" ); - nr_cpus = nr_cpus >8 ? nr_cpus -8 :0 ; - } - } +print_bitmap (vcpuinfo->cpumap.map, nr_cpus, stdout); +printf ("\n" ); }static void print_domain_vcpuinfo (uint32_t domid,uint32_t nr_cpus)
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 21 lines · Andre Przywara
Hi,
# xl vcpu-list hangs on my big box. The issue is an endless loop, where the algorithm for printing the CPU affinity in a condensed way looks for a set bit in a zero-byte: for (i = 0; !(pcpumap & 1); ++i, pcpumap >>= 1) Looking at the code I found that it is entirely broken if more than 8 CPUs are used. Beside that endless loop issue the output is totally bogus except for the "any CPU" case, which is handled explicitly earlier. I tried to fix it, but the whole approach does not work if the outer loops actually iterates (executing more than once). I could not copy the Linux version of that algorithm due to licensing incompatibilities and the Python version is not easily converted to C, so I coded my own version from scratch. It is a bit verbose since it iterates over bits instead of bytes, but more cleaner and survived some unit-testing. I didn't spend much time in optimizing it, though. I put it in a separate function as I plan to use it later for printing cpupool affinity in a similar way (a post 4.1.0 patch living in one of my branches).
Quoted 6 lines
If you have a better implementation available, I can push it through my automated unit test easily.
Please review and apply to Xen 4.1.0-rc.
Signed-off-by Andre Przywara <[email protected]>
Thank you very much for fixing this, also I like the implementation!
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: fix broken xl vcpu-list output"):
Quoted 4 lines · Andre Przywara
Quoted 1 line
Signed-off-by Andre Przywara <[email protected]>Thank you very much for fixing this, also I like the implementation!
Indeed, well spotted and thanks. Applied.
Ian.