Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
So, it appears that the only change that we agree on for now is the change to m5.c. Should I submit that change as its own patch and withdraw this one? Thanks, Joel On Fri, Jul 23, 2010 at 3:45 PM, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Quoting Ali Saidi sa...@umich.edu: On Fri, 23 Jul 2010 16:59:08 -0400, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? It was reserved for me while I was doing the bottleneck analysis work and didn't want anyone to grab that ID. Once I pushed all of the bottleneck analysis changes, I changed reserved into the actual cp_annotate operations. So, everything worked as intended. reserved1_func shouldn't be used anywhere and shouldn't be added back to the file. Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev I don't understand how that made it reserved. Wouldn't anyone else be able to do the same thing you did but with some conflicting use? The comment next to those says Reserved for user, but it's not if it ends up being assigned an official use. Why would we want to have reserved2_func but not reserved1_func? Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev -- Joel Hestness PhD Student, Computer Architecture Dept. of Computer Science, University of Texas - Austin http://www.cs.utexas.edu/~hestness ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
Actually I have a minor comment there too. You should really only need to touch a byte on each page of the buffer, not all bytes like memset would. This will make a read_file take longer than it absolutely needs to, although it's likely negligable. Feel free to ignore this as you see fit, perhaps after giving it a try to see if the difference is even perceivable. Gabe Quoting Joel Hestness hestn...@cs.utexas.edu: So, it appears that the only change that we agree on for now is the change to m5.c. Should I submit that change as its own patch and withdraw this one? Thanks, Joel On Fri, Jul 23, 2010 at 3:45 PM, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Quoting Ali Saidi sa...@umich.edu: On Fri, 23 Jul 2010 16:59:08 -0400, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? It was reserved for me while I was doing the bottleneck analysis work and didn't want anyone to grab that ID. Once I pushed all of the bottleneck analysis changes, I changed reserved into the actual cp_annotate operations. So, everything worked as intended. reserved1_func shouldn't be used anywhere and shouldn't be added back to the file. Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev I don't understand how that made it reserved. Wouldn't anyone else be able to do the same thing you did but with some conflicting use? The comment next to those says Reserved for user, but it's not if it ends up being assigned an official use. Why would we want to have reserved2_func but not reserved1_func? Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev -- Joel Hestness PhD Student, Computer Architecture Dept. of Computer Science, University of Texas - Austin http://www.cs.utexas.edu/~hestness ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
Joel, The reserved1_func shouldn't be added back to the header file. The that id was reserved by me while I was working on the critical path analysis stuff and is now used by that. The comment needs a bit of work. I shouldn't have said, reserved for user, but rather, reserved by a specific user for future functionality, do not use! The m5.c change is fine with me. I can think of reasons to statically compile the binary and reasons not to. Either way is fine with me unless someone is vehemently opposed. Thanks, Ali On Jul 29, 2010, at 7:45 PM, Gabriel Michael Black wrote: Actually I have a minor comment there too. You should really only need to touch a byte on each page of the buffer, not all bytes like memset would. This will make a read_file take longer than it absolutely needs to, although it's likely negligable. Feel free to ignore this as you see fit, perhaps after giving it a try to see if the difference is even perceivable. Gabe Quoting Joel Hestness hestn...@cs.utexas.edu: So, it appears that the only change that we agree on for now is the change to m5.c. Should I submit that change as its own patch and withdraw this one? Thanks, Joel On Fri, Jul 23, 2010 at 3:45 PM, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Quoting Ali Saidi sa...@umich.edu: On Fri, 23 Jul 2010 16:59:08 -0400, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? It was reserved for me while I was doing the bottleneck analysis work and didn't want anyone to grab that ID. Once I pushed all of the bottleneck analysis changes, I changed reserved into the actual cp_annotate operations. So, everything worked as intended. reserved1_func shouldn't be used anywhere and shouldn't be added back to the file. Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev I don't understand how that made it reserved. Wouldn't anyone else be able to do the same thing you did but with some conflicting use? The comment next to those says Reserved for user, but it's not if it ends up being assigned an official use. Why would we want to have reserved2_func but not reserved1_func? Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev -- Joel Hestness PhD Student, Computer Architecture Dept. of Computer Science, University of Texas - Austin http://www.cs.utexas.edu/~hestness ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
Hey Gabe, Comments are in-lined below. If you'd like me to resubmit another review of all or part, just let me know. Thanks, Joel util/m5/Makefile.x86 http://reviews.m5sim.org/r/64/#comment248 Why is this necessary? Is this so it runs under SE mode? In that case I think we should make it run like before as the default since 99% of the time this will run in FS, and provide a way to inject -static for the 1% of the time it runs in SE. Compiling it as static all the time wouldn't be the end of the world, but it seems like we'd be making universal changes for a very uncommon case. Building the m5 binary without -static allows it to dynamically link a few libraries: j...@capillary:~/research/m5-new/util/m5$ ldd m5 linux-vdso.so.1 = (0x7fff3f9ff000) libc.so.6 = /lib/libc.so.6 (0x7fb05131f000) /lib64/ld-linux-x86-64.so.2 (0x7fb05168f000) When I was putting together a disk image using busybox, it had issues with library versions. In general, since the m5 utility isn't performance critical and just implements simulator magic, I think it would be easiest if it was always built statically whether for FS or SE. On the other, I would imagine that it's built very infrequently and only for initial disk image creation, so perhaps its not worth changing. util/m5/m5ops.h http://reviews.m5sim.org/r/64/#comment249 It looks like Ali commandeered that value on line 61. It might have been better to use 0x5A for that, but it also might not be safe to change it now since there may be binaries out there that use it (probably not too many). It would be a little strange, but you could actually use 0x5A for reserved1_func. I don't know what restrictions there are in the various ISAs for function numbers, but in x86 it's a 16 bit value. Ah, I didn't see that originally! The only real trouble right now is that if you try to build the m5 utility for x86_64 with the current version in the repo, it will fail with an undefined reference to reserved1_func: gcc -O2 -o m5op_x86.o -c m5op_x86.S gcc -o m5 m5.o m5op_x86.o m5op_x86.o: In function `m5_reserved1_func': (.text+0x5c): undefined reference to `reserved1_func' collect2: ld returned 1 exit status make: *** [m5] Error 1 It looks like neither m5op_alpha.S or m5op_sparc.S use reserved1_func, so another solution would be to remove it from m5op_x86.S (eliminate it completely from the m5 utility codebase). - Gabe -- Joel Hestness PhD Student, Computer Architecture Dept. of Computer Science, University of Texas - Austin http://www.cs.utexas.edu/~hestness ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
Quoting Joel Hestness hestn...@cs.utexas.edu: Hey Gabe, Comments are in-lined below. If you'd like me to resubmit another review of all or part, just let me know. Thanks, Joel util/m5/Makefile.x86 http://reviews.m5sim.org/r/64/#comment248 Why is this necessary? Is this so it runs under SE mode? In that case I think we should make it run like before as the default since 99% of the time this will run in FS, and provide a way to inject -static for the 1% of the time it runs in SE. Compiling it as static all the time wouldn't be the end of the world, but it seems like we'd be making universal changes for a very uncommon case. Building the m5 binary without -static allows it to dynamically link a few libraries: j...@capillary:~/research/m5-new/util/m5$ ldd m5 linux-vdso.so.1 = (0x7fff3f9ff000) libc.so.6 = /lib/libc.so.6 (0x7fb05131f000) /lib64/ld-linux-x86-64.so.2 (0x7fb05168f000) When I was putting together a disk image using busybox, it had issues with library versions. In general, since the m5 utility isn't performance critical and just implements simulator magic, I think it would be easiest if it was always built statically whether for FS or SE. On the other, I would imagine that it's built very infrequently and only for initial disk image creation, so perhaps its not worth changing. You have a good reason to build it statically, but it would still be best, I think, if that was optional. I'm not sure what the best way to do that would be. Maybe another target? Leaving LDFLAGS in the command line but not setting it to anything might work too, although then there's the danger of picking something up from the environment. I don't think it's worth being too elaborate for. Another question might be why we have three different nearly identical make files instead of one, or maybe one with really short ones for each arch, or even just pulling it into scons. Maybe to avoid complicating things too much this would be its own little scons environment independent from the main one. I think there are enough unanswered (but not serious) questions here that we might want to wait to check this part in. util/m5/m5ops.h http://reviews.m5sim.org/r/64/#comment249 It looks like Ali commandeered that value on line 61. It might have been better to use 0x5A for that, but it also might not be safe to change it now since there may be binaries out there that use it (probably not too many). It would be a little strange, but you could actually use 0x5A for reserved1_func. I don't know what restrictions there are in the various ISAs for function numbers, but in x86 it's a 16 bit value. Ah, I didn't see that originally! The only real trouble right now is that if you try to build the m5 utility for x86_64 with the current version in the repo, it will fail with an undefined reference to reserved1_func: gcc -O2 -o m5op_x86.o -c m5op_x86.S gcc -o m5 m5.o m5op_x86.o m5op_x86.o: In function `m5_reserved1_func': (.text+0x5c): undefined reference to `reserved1_func' collect2: ld returned 1 exit status make: *** [m5] Error 1 It looks like neither m5op_alpha.S or m5op_sparc.S use reserved1_func, so another solution would be to remove it from m5op_x86.S (eliminate it completely from the m5 utility codebase). Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
On Fri, 23 Jul 2010 16:59:08 -0400, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? It was reserved for me while I was doing the bottleneck analysis work and didn't want anyone to grab that ID. Once I pushed all of the bottleneck analysis changes, I changed reserved into the actual cp_annotate operations. So, everything worked as intended. reserved1_func shouldn't be used anywhere and shouldn't be added back to the file. Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
Quoting Ali Saidi sa...@umich.edu: On Fri, 23 Jul 2010 16:59:08 -0400, Gabriel Michael Black gbl...@eecs.umich.edu wrote: Hmm, maybe we should be building these regularly too... What do you think, Ali? Would it be possible to return reserved1_func and use a different code? It was reserved for me while I was doing the bottleneck analysis work and didn't want anyone to grab that ID. Once I pushed all of the bottleneck analysis changes, I changed reserved into the actual cp_annotate operations. So, everything worked as intended. reserved1_func shouldn't be used anywhere and shouldn't be added back to the file. Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev I don't understand how that made it reserved. Wouldn't anyone else be able to do the same thing you did but with some conflicting use? The comment next to those says Reserved for user, but it's not if it ends up being assigned an official use. Why would we want to have reserved2_func but not reserved1_func? Gabe ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/64/#review106 --- util/m5/Makefile.x86 http://reviews.m5sim.org/r/64/#comment248 Why is this necessary? Is this so it runs under SE mode? In that case I think we should make it run like before as the default since 99% of the time this will run in FS, and provide a way to inject -static for the 1% of the time it runs in SE. Compiling it as static all the time wouldn't be the end of the world, but it seems like we'd be making universal changes for a very uncommon case. util/m5/m5ops.h http://reviews.m5sim.org/r/64/#comment249 It looks like Ali commandeered that value on line 61. It might have been better to use 0x5A for that, but it also might not be safe to change it now since there may be binaries out there that use it (probably not too many). It would be a little strange, but you could actually use 0x5A for reserved1_func. I don't know what restrictions there are in the various ISAs for function numbers, but in x86 it's a 16 bit value. - Gabe On 2010-07-21 14:57:19, Joel Hestness wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/64/ --- (Updated 2010-07-21 14:57:19) Review request for Default. Summary --- util/m5/m5.c: in readfile(), added memset to touch all pages - ensure they are in the page table util/m5/m5ops/h: add reserved1_func back into list of magic ops util/m5/Makefile.x86: should build the binary statically Diffs - util/m5/Makefile.x86 a75564db03c3 util/m5/m5.c a75564db03c3 util/m5/m5ops.h a75564db03c3 Diff: http://reviews.m5sim.org/r/64/diff Testing --- Thanks, Joel ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
