Messages in this thread Patch in this message | | | Date | Tue, 14 Dec 2010 15:21:09 -0500 | From | Jeff Mahoney <> | Subject | Re: [patch] delayacct: fix iotop on x86_64 |
| |
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 12/14/2010 03:16 PM, Andrew Morton wrote: > On Tue, 14 Dec 2010 13:32:39 +0530 > Balbir Singh <balbir@linux.vnet.ibm.com> wrote: > >> * Dan Carpenter <error27@gmail.com> [2010-12-14 10:02:43]: >> >>> We changed how the taskstats was exported to user space in: >>> 85893120699 "delayacct: align to 8 byte boundary on 64-bit systems" >>> This was important because it fixes a run time warning on IA64. In >>> theory it shouldn't have broken anything, if you just assume that user >>> space programmers don't smoke crack all day long. >>> >>> But actually it breaks iotop on x86_64. >>> >>> Reported-by: Brian Rogers <brian@xyzw.org> >>> Signed-off-by: Dan Carpenter <error27@gmail.com> >>> >>> diff --git a/kernel/taskstats.c b/kernel/taskstats.c >>> index c8231fb..a0758de 100644 >>> --- a/kernel/taskstats.c >>> +++ b/kernel/taskstats.c >>> @@ -358,7 +358,19 @@ static struct taskstats *mk_reply(struct sk_buff *skb, int type, u32 pid) >>> * This causes lots of runtime warnings on systems requiring 8 byte >>> * alignment */ >>> u32 pids[2] = { pid, 0 }; >>> - int pid_size = ALIGN(sizeof(pid), sizeof(long)); >>> + int pid_size; >>> + >>> + /* >>> + * IA64 can't be aligned on a 4 byte boundary. But iotop on x86_64 >>> + * depends on the current struct layout. The next version of iotop >>> + * will fix this so maybe we can move everything to the new code in >>> + * a couple years. >>> + */ >>> +#if defined(CONFIG_IA64) >>> + pid_size = ALIGN(sizeof(pid), sizeof(long)); >>> +#else >>> + pid_size = sizeof(u32); >>> +#endif >> >> I would rather abstract this better > > Well. Abstracting something tends to make it permanent. When you have > an ugly, special-case temporary hack, there is merit to having it > sitting there in the middle of the code staring you in the face. It's > very explicit and we won't forget about it. > >> and I'd be apprehensive about the >> fix if iotop was at fault to begin with, I would rather fix iotop. >> IOW, are we fixing what iotop got wrong? Isn't it easier to backport >> the correct behaviour in iotop. I understand we broke the ABI, but >> user space can still live. > > Nah, let's not knowingly break a userspace app. > > > This is a versioned interface, is it not? How is that supposed > to work? Should we have upped the version number when making this > change?
Perhaps. Balbir suggested it, but it didn't make it into the final version. Not that it would have mattered. iotop doesn't look at the version anyway.
- -Jeff
- -- Jeff Mahoney SUSE Labs -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/
iEYEARECAAYFAk0H0bUACgkQLPWxlyuTD7LJlwCeKLRuVKXIwZi7XMARDNXmBxkj QC0An0up3AVv/G8T8JZbb+cpDFagKnj0 =ra4a -----END PGP SIGNATURE-----
| |