Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
On 2011-03-18 13:57:35, Gabe Black wrote: src/sim/syscall_emul.hh, line 503 http://reviews.m5sim.org/r/589/diff/1/?file=11013#file11013line503 Why is this change necessary? I'm not 100% sure why it was the way it was before, but I see no reason to change it either. Changing the fatal to a warn may be necessary to get some benchmark to run, but I'm talking specifically about the ones that would return -ENOTTY. Vince Weaver wrote: It's been a while, but let's see if I can see what's going on. Before we had a short list of ioctls we return -ENOTTY for, any not on the list gave a fatal error. ENOTTY if I recall correctly is the typical way the kernel reports an ioctl not being implemented. So by changing all ioctls to warn and then report ENOTTY we are saying that we don't support it, but the program can keep running assuming it handles an ENOTTY properly. For most of the SPEC benchmarks ioctl() calls aren't important for correctness, so this works. We could go back to having a whitelist of known-to-be-OK-to-ignore ioctls(), but it might turn out to be a lengthy list, and also we probably should implement things like isatty() properly as I do think some benchmarks depend on it returning the right value (see http://www.cs.binghamton.edu/~msim/wiki/index.php/TIOCISATTY ) Gabe Black wrote: isatty is actually pretty annoying, and it's important that it -not- always return the right answer. It needs to be consistent regardless of whether your piping output to a file or watching it on the console so runs are deterministic, and if it always thinks it's going to a tty it'll buffer properly for when it's actually going to a tty (and doesn't look like something's broken, basically) and just perform a little worse when it's not. As far as simulator performance it doesn't matter since I'm sure it's a long way from being the critical path, and it should be a reasonable situation as far as simulated performance. I think a whitelist is a good idea, and we don't have to have -all- the ok to ignore ones on there, just the ok to ignore ones that actually get called. That should be a lot more manageable list. That way you know if something out of the ordinary is happening (the simulator will bomb out) rather than something weird happening and the benchmark stumbling on to eventually die, potentially far from evidence of what happened. The later is a lot harder to debug. Steve Reinhardt wrote: ENOTTY is just the way the kernel reports a TTY-specific ioctl being invoked on a non-TTY device. Gabe is right that for repeatability you always want the simulator to always act like there's no tty for determinism. I would prefer to see us figure out what other ioctls need to be added to the TTY-specific list than to make a blanket assumption that any ioctl that's called is TTY-specific. Lisa Hsu wrote: So, what's the consensus here? No checkin until ioctls are better figured out? The change to syscalls.cc is fine since that's just hooking up the existing syscall functions and there isn't anything unusual or objectionable about how that's being done. The change to syscall_emul.hh is not correct as far as I can tell. If there are specific IOCTL constants that are being used, we'll need to find out what those are and then what to do with them. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/#review981 --- On 2011-03-17 16:06:13, Lisa Hsu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- (Updated 2011-03-17 16:06:13) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
On 2011-03-18 13:57:35, Gabe Black wrote: src/sim/syscall_emul.hh, line 503 http://reviews.m5sim.org/r/589/diff/1/?file=11013#file11013line503 Why is this change necessary? I'm not 100% sure why it was the way it was before, but I see no reason to change it either. Changing the fatal to a warn may be necessary to get some benchmark to run, but I'm talking specifically about the ones that would return -ENOTTY. Vince Weaver wrote: It's been a while, but let's see if I can see what's going on. Before we had a short list of ioctls we return -ENOTTY for, any not on the list gave a fatal error. ENOTTY if I recall correctly is the typical way the kernel reports an ioctl not being implemented. So by changing all ioctls to warn and then report ENOTTY we are saying that we don't support it, but the program can keep running assuming it handles an ENOTTY properly. For most of the SPEC benchmarks ioctl() calls aren't important for correctness, so this works. We could go back to having a whitelist of known-to-be-OK-to-ignore ioctls(), but it might turn out to be a lengthy list, and also we probably should implement things like isatty() properly as I do think some benchmarks depend on it returning the right value (see http://www.cs.binghamton.edu/~msim/wiki/index.php/TIOCISATTY ) Gabe Black wrote: isatty is actually pretty annoying, and it's important that it -not- always return the right answer. It needs to be consistent regardless of whether your piping output to a file or watching it on the console so runs are deterministic, and if it always thinks it's going to a tty it'll buffer properly for when it's actually going to a tty (and doesn't look like something's broken, basically) and just perform a little worse when it's not. As far as simulator performance it doesn't matter since I'm sure it's a long way from being the critical path, and it should be a reasonable situation as far as simulated performance. I think a whitelist is a good idea, and we don't have to have -all- the ok to ignore ones on there, just the ok to ignore ones that actually get called. That should be a lot more manageable list. That way you know if something out of the ordinary is happening (the simulator will bomb out) rather than something weird happening and the benchmark stumbling on to eventually die, potentially far from evidence of what happened. The later is a lot harder to debug. Steve Reinhardt wrote: ENOTTY is just the way the kernel reports a TTY-specific ioctl being invoked on a non-TTY device. Gabe is right that for repeatability you always want the simulator to always act like there's no tty for determinism. I would prefer to see us figure out what other ioctls need to be added to the TTY-specific list than to make a blanket assumption that any ioctl that's called is TTY-specific. So, what's the consensus here? No checkin until ioctls are better figured out? - Lisa --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/#review981 --- On 2011-03-17 16:06:13, Lisa Hsu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- (Updated 2011-03-17 16:06:13) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/#review981 --- src/sim/syscall_emul.hh http://reviews.m5sim.org/r/589/#comment1357 Why is this change necessary? I'm not 100% sure why it was the way it was before, but I see no reason to change it either. Changing the fatal to a warn may be necessary to get some benchmark to run, but I'm talking specifically about the ones that would return -ENOTTY. - Gabe On 2011-03-17 16:06:13, Lisa Hsu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- (Updated 2011-03-17 16:06:13) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
On 2011-03-18 13:57:35, Gabe Black wrote: src/sim/syscall_emul.hh, line 503 http://reviews.m5sim.org/r/589/diff/1/?file=11013#file11013line503 Why is this change necessary? I'm not 100% sure why it was the way it was before, but I see no reason to change it either. Changing the fatal to a warn may be necessary to get some benchmark to run, but I'm talking specifically about the ones that would return -ENOTTY. Vince Weaver wrote: It's been a while, but let's see if I can see what's going on. Before we had a short list of ioctls we return -ENOTTY for, any not on the list gave a fatal error. ENOTTY if I recall correctly is the typical way the kernel reports an ioctl not being implemented. So by changing all ioctls to warn and then report ENOTTY we are saying that we don't support it, but the program can keep running assuming it handles an ENOTTY properly. For most of the SPEC benchmarks ioctl() calls aren't important for correctness, so this works. We could go back to having a whitelist of known-to-be-OK-to-ignore ioctls(), but it might turn out to be a lengthy list, and also we probably should implement things like isatty() properly as I do think some benchmarks depend on it returning the right value (see http://www.cs.binghamton.edu/~msim/wiki/index.php/TIOCISATTY ) isatty is actually pretty annoying, and it's important that it -not- always return the right answer. It needs to be consistent regardless of whether your piping output to a file or watching it on the console so runs are deterministic, and if it always thinks it's going to a tty it'll buffer properly for when it's actually going to a tty (and doesn't look like something's broken, basically) and just perform a little worse when it's not. As far as simulator performance it doesn't matter since I'm sure it's a long way from being the critical path, and it should be a reasonable situation as far as simulated performance. I think a whitelist is a good idea, and we don't have to have -all- the ok to ignore ones on there, just the ok to ignore ones that actually get called. That should be a lot more manageable list. That way you know if something out of the ordinary is happening (the simulator will bomb out) rather than something weird happening and the benchmark stumbling on to eventually die, potentially far from evidence of what happened. The later is a lot harder to debug. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/#review981 --- On 2011-03-17 16:06:13, Lisa Hsu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- (Updated 2011-03-17 16:06:13) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
On 2011-03-18 13:57:35, Gabe Black wrote: src/sim/syscall_emul.hh, line 503 http://reviews.m5sim.org/r/589/diff/1/?file=11013#file11013line503 Why is this change necessary? I'm not 100% sure why it was the way it was before, but I see no reason to change it either. Changing the fatal to a warn may be necessary to get some benchmark to run, but I'm talking specifically about the ones that would return -ENOTTY. Vince Weaver wrote: It's been a while, but let's see if I can see what's going on. Before we had a short list of ioctls we return -ENOTTY for, any not on the list gave a fatal error. ENOTTY if I recall correctly is the typical way the kernel reports an ioctl not being implemented. So by changing all ioctls to warn and then report ENOTTY we are saying that we don't support it, but the program can keep running assuming it handles an ENOTTY properly. For most of the SPEC benchmarks ioctl() calls aren't important for correctness, so this works. We could go back to having a whitelist of known-to-be-OK-to-ignore ioctls(), but it might turn out to be a lengthy list, and also we probably should implement things like isatty() properly as I do think some benchmarks depend on it returning the right value (see http://www.cs.binghamton.edu/~msim/wiki/index.php/TIOCISATTY ) Gabe Black wrote: isatty is actually pretty annoying, and it's important that it -not- always return the right answer. It needs to be consistent regardless of whether your piping output to a file or watching it on the console so runs are deterministic, and if it always thinks it's going to a tty it'll buffer properly for when it's actually going to a tty (and doesn't look like something's broken, basically) and just perform a little worse when it's not. As far as simulator performance it doesn't matter since I'm sure it's a long way from being the critical path, and it should be a reasonable situation as far as simulated performance. I think a whitelist is a good idea, and we don't have to have -all- the ok to ignore ones on there, just the ok to ignore ones that actually get called. That should be a lot more manageable list. That way you know if something out of the ordinary is happening (the simulator will bomb out) rather than something weird happening and the benchmark stumbling on to eventually die, potentially far from evidence of what happened. The later is a lot harder to debug. ENOTTY is just the way the kernel reports a TTY-specific ioctl being invoked on a non-TTY device. Gabe is right that for repeatability you always want the simulator to always act like there's no tty for determinism. I would prefer to see us figure out what other ioctls need to be added to the TTY-specific list than to make a blanket assumption that any ioctl that's called is TTY-specific. - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/#review981 --- On 2011-03-17 16:06:13, Lisa Hsu wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- (Updated 2011-03-17 16:06:13) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc 2e269d6fb3e6 src/sim/syscall_emul.hh 2e269d6fb3e6 Diff: http://reviews.m5sim.org/r/589/diff Testing --- Thanks, Lisa ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
I assume that when you commit these, you'll put a proper message in and make the author vince (qref -u can set the username) Nate On Thu, Mar 17, 2011 at 4:06 PM, Lisa Hsu h...@eecs.umich.edu wrote: This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. By Lisa Hsu. Description X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc (2e269d6fb3e6) - src/sim/syscall_emul.hh (2e269d6fb3e6) View Diff http://reviews.m5sim.org/r/589/diff/ ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: X86 ioctl: Another patch from Vince Weaver
Yes, you assume right. On Thu, Mar 17, 2011 at 4:08 PM, nathan binkert n...@binkert.org wrote: I assume that when you commit these, you'll put a proper message in and make the author vince (qref -u can set the username) Nate On Thu, Mar 17, 2011 at 4:06 PM, Lisa Hsu h...@eecs.umich.edu wrote: This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/589/ Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. By Lisa Hsu. Description X86 ioctl: Another patch from Vince Weaver Diffs - src/arch/x86/linux/syscalls.cc (2e269d6fb3e6) - src/sim/syscall_emul.hh (2e269d6fb3e6) View Diff http://reviews.m5sim.org/r/589/diff/ ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
