lkml.org 
[lkml]   [2010]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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-----

\
 
 \ /
  Last update: 2010-12-14 21:23    [W:0.086 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site