Re: [patch] delayacct: fix iotop on x86_64

[Balbir Singh]

* Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> [2010-12-14 12:16:41]:

> On Tue, 14 Dec 2010 13:32:39 +0530
> Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > * Dan Carpenter <error27@xxxxxxxxx> [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@xxxxxxxx>
> > > Signed-off-by: Dan Carpenter <error27@xxxxxxxxx>
> > > 
> > > 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.
>

OK, agreed and learnt
 
> > 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.
>

Fair enough!
 
> 
> 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?

-- 
	Three Cheers,
	Balbir
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/