[PATCH] (18/22) task_thread_info - part 2/4

[PATCH] (18/22) task_thread_info - part 2/4

[Al Viro]

encapsulates the rest of arch-dependent operations with thread_info access.
Two new helpers - setup_thread_info() and end_of_stack().  For normal
case the former consists of copying thread_info of parent to new thread_info
and the latter returns pointer immediately past the end of thread_info.

Signed-off-by: Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
----
diff -urN RC13-rc7-task_thread_info/include/linux/sched.h RC13-rc7-other-helpers/include/linux/sched.h
--- RC13-rc7-task_thread_info/include/linux/sched.h	2005-08-25 00:54:17.000000000 -0400
+++ RC13-rc7-other-helpers/include/linux/sched.h	2005-08-25 00:54:17.000000000 -0400
@@ -1138,6 +1138,16 @@
 
 #define task_thread_info(task) (task)->thread_info
 
+static inline void setup_thread_info(struct task_struct *p, struct thread_info *ti)
+{
+	*ti = *p->thread_info;
+}
+
+static inline unsigned long *end_of_stack(struct task_struct *p)
+{
+	return (unsigned long *)(p->thread_info + 1);
+}
+
 /* set thread flags in other task's structures
  * - see asm/thread_info.h for TIF_xxxx flags available
  */
diff -urN RC13-rc7-task_thread_info/kernel/fork.c RC13-rc7-other-helpers/kernel/fork.c
--- RC13-rc7-task_thread_info/kernel/fork.c	2005-08-25 00:54:17.000000000 -0400
+++ RC13-rc7-other-helpers/kernel/fork.c	2005-08-25 00:54:17.000000000 -0400
@@ -169,8 +169,8 @@
 		return NULL;
 	}
 
-	*ti = *orig->thread_info;
 	*tsk = *orig;
+	setup_thread_info(tsk, ti);
 	tsk->thread_info = ti;
 	ti->task = tsk;
 
diff -urN RC13-rc7-task_thread_info/kernel/sched.c RC13-rc7-other-helpers/kernel/sched.c
--- RC13-rc7-task_thread_info/kernel/sched.c	2005-08-25 00:54:17.000000000 -0400
+++ RC13-rc7-other-helpers/kernel/sched.c	2005-08-25 00:54:17.000000000 -0400
@@ -4121,10 +4121,10 @@
 #endif
 #ifdef CONFIG_DEBUG_STACK_USAGE
 	{
-		unsigned long * n = (unsigned long *) (p->thread_info+1);
+		unsigned long * n = end_of_stack(p);
 		while (!*n)
 			n++;
-		free = (unsigned long) n - (unsigned long)(p->thread_info+1);
+		free = (unsigned long) n - (unsigned long) end_of_stack(p);
 	}
 #endif
 	printk("%5lu %5d %6d ", free, p->pid, p->parent->pid);

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Geert Uytterhoeven]

Thanks a lot, Al!

On Thu, 25 Aug 2005, Al Viro wrote:
> encapsulates the rest of arch-dependent operations with thread_info access.
> Two new helpers - setup_thread_info() and end_of_stack().  For normal
> case the former consists of copying thread_info of parent to new thread_info
> and the latter returns pointer immediately past the end of thread_info.
> 
> Signed-off-by: Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
> ----
> diff -urN RC13-rc7-task_thread_info/include/linux/sched.h RC13-rc7-other-helpers/include/linux/sched.h
> --- RC13-rc7-task_thread_info/include/linux/sched.h	2005-08-25 00:54:17.000000000 -0400
> +++ RC13-rc7-other-helpers/include/linux/sched.h	2005-08-25 00:54:17.000000000 -0400
> @@ -1138,6 +1138,16 @@
>  
>  #define task_thread_info(task) (task)->thread_info
>  
> +static inline void setup_thread_info(struct task_struct *p, struct thread_info *ti)
					^^^^^^^^^^^^^^^^^^^^^
					const struct task_struct *p?

> +{
> +	*ti = *p->thread_info;
> +}
> +
> +static inline unsigned long *end_of_stack(struct task_struct *p)
					     ^^^^^^^^^^^^^^^^^^^^^
					     const struct task_struct *p?

> +{
> +	return (unsigned long *)(p->thread_info + 1);
> +}
> +

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Al Viro]

On Thu, Aug 25, 2005 at 10:41:27AM +0200, Geert Uytterhoeven wrote:
> >  
> > +static inline void setup_thread_info(struct task_struct *p, struct thread_info *ti)
> 					^^^^^^^^^^^^^^^^^^^^^
> 					const struct task_struct *p?

Umm...  Wouldn't be a problem, but what does it buy?  The only caller
has p very much _not_ const pointer and since it's inlined, you won't
gain any optimizations in the caller out of it.
 
> > +{
> > +	*ti = *p->thread_info;
> > +}
> > +
> > +static inline unsigned long *end_of_stack(struct task_struct *p)
> 					     ^^^^^^^^^^^^^^^^^^^^^
> 					     const struct task_struct *p?

That one is OK, but then you really want const pointer out of it.

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Roman Zippel]

Hi,

On Thu, 25 Aug 2005, Al Viro wrote:

>  
> +static inline void setup_thread_info(struct task_struct *p, struct thread_info *ti)
> +{
> +	*ti = *p->thread_info;
> +}
> +
>  
> -	*ti = *orig->thread_info;
>  	*tsk = *orig;
> +	setup_thread_info(tsk, ti);
>  	tsk->thread_info = ti;
>  	ti->task = tsk;

This introduces a subtle ordering requirement, where setup_thread_info 
magically finds in the new task_struct the pointer to the old thread_info 
to setup the new thread_info.
What is your problem with what I have in CVS? There it completes the basic
task_struct setup and _after_ that it can setup the thread_info.

Al, I would really prefer to merge this one myself, I'm only waiting for 
the 2.6.13 release and since this is not a regression, I don't really 
understand why this must be in 2.6.13.

bye, Roman

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Al Viro]

On Thu, Aug 25, 2005 at 11:15:24AM +0200, Roman Zippel wrote:
> >  
> > -	*ti = *orig->thread_info;
> >  	*tsk = *orig;
> > +	setup_thread_info(tsk, ti);
> >  	tsk->thread_info = ti;
> >  	ti->task = tsk;
> 
> This introduces a subtle ordering requirement, where setup_thread_info 
> magically finds in the new task_struct the pointer to the old thread_info 
> to setup the new thread_info.

Nothing subtle with it, especially since this is the only place with any
business to call setup_thread_info().

> What is your problem with what I have in CVS? There it completes the basic
> task_struct setup and _after_ that it can setup the thread_info.

Which buys you what, exactly?  You end up with more things to do in
setup_thread_info() and it doesn't get cleaner.

> Al, I would really prefer to merge this one myself, I'm only waiting for 
> the 2.6.13 release and since this is not a regression, I don't really 
> understand why this must be in 2.6.13.

Fine, as long as that merge is done before your s/thread_info/stack/ patches.
It should be the first step before doing 200Kb worth of cosmetical stuff
that affects every architecture out there, not something that depends on
it done.

There's also a question of having mainline build and work on the architecture
in question, which obviously is not something you care about - this hairball
had been sitting in m68k CVS for how long?  Since 2.5.60-something, with
zero efforts to resolve it, right?  And mainline kernel didn't even build,
let alone work since that moment.

FWIW, essentially the same splitup of that mess had been posted more than
three months ago; definitely before 2.6.12-final.  Still no activity _and_
plans that involve doing kernel-wide renaming of struct thread_info *
thread_info in task_struct to void *stack as part of m68k merge.  With
200-odd Kb of patches just out of that renaming.  At which point I gave
up on explaining the difference between "take the diff between mainline
and CVS + whatever needed to make all other platforms compile after change
and try to shove it into mainline" and "do minimally intrusive merge,
followed by sane cleanup sequence done in mainline".

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Christoph Hellwig]

On Thu, Aug 25, 2005 at 02:07:38PM +0100, Al Viro wrote:
> Fine, as long as that merge is done before your s/thread_info/stack/ patches.
> It should be the first step before doing 200Kb worth of cosmetical stuff
> that affects every architecture out there, not something that depends on
> it done.
> 
> There's also a question of having mainline build and work on the architecture
> in question, which obviously is not something you care about - this hairball
> had been sitting in m68k CVS for how long?  Since 2.5.60-something, with
> zero efforts to resolve it, right?  And mainline kernel didn't even build,
> let alone work since that moment.

Yup.  Let's get m68k into buildable shape for 2.6.13 with Al's minimal
patches, and if you have further improvements over that submit them as
split up patches through the usual channels.  Having all architectures
actually build and work from mainline is really important to have
useful kernel package in distributions.

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Roman Zippel]

Hi,

On Thu, 25 Aug 2005, Christoph Hellwig wrote:

> Yup.  Let's get m68k into buildable shape for 2.6.13 with Al's minimal
> patches, and if you have further improvements over that submit them as
> split up patches through the usual channels.  Having all architectures
> actually build and work from mainline is really important to have
> useful kernel package in distributions.

No, there has been no discussion of these patches, so there is no point in 
doing this a few days before 2.6.13. Can we please do this properly for 
2.6.14?

If you want to apply these patches, please also apply the following patch:

Index: linux-2.6/MAINTAINERS
===================================================================
--- linux-2.6.orig/MAINTAINERS	2005-08-10 01:45:52.000000000 +0200
+++ linux-2.6/MAINTAINERS	2005-08-25 16:05:41.798908837 +0200
@@ -1511,8 +1511,8 @@ S:	Maintained
 M68K ARCHITECTURE
 P:	Geert Uytterhoeven
 M:	geert@linux-m68k.org
-P:	Roman Zippel
-M:	zippel@linux-m68k.org
+P:	Al Viro
+M:	viro@parcelfarce.linux.theplanet.co.uk
 L:	linux-m68k@lists.linux-m68k.org
 W:	http://www.linux-m68k.org/
 W:	http://linux-m68k-cvs.ubb.ca/


I'm serious about this.

bye, Roman

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Geert Uytterhoeven]

On Thu, 25 Aug 2005, Roman Zippel wrote:
> On Thu, 25 Aug 2005, Christoph Hellwig wrote:
> > Yup.  Let's get m68k into buildable shape for 2.6.13 with Al's minimal
> > patches, and if you have further improvements over that submit them as
> > split up patches through the usual channels.  Having all architectures
> > actually build and work from mainline is really important to have
> > useful kernel package in distributions.
> 
> No, there has been no discussion of these patches, so there is no point in 
> doing this a few days before 2.6.13. Can we please do this properly for 
> 2.6.14?

Notwithstanding the actual content of the patches, I agree there's indeed no
need to hurry (unless Christoph's unifying Debian kernel hat is weighting a
lot).

A few months of delay for 2.6.14 is almost unnoticeable on the Linux/m68k
timescale anyway ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Al Viro]

On Thu, Aug 25, 2005 at 04:15:39PM +0200, Roman Zippel wrote:
> Hi,
> 
> On Thu, 25 Aug 2005, Christoph Hellwig wrote:
> 
> > Yup.  Let's get m68k into buildable shape for 2.6.13 with Al's minimal
> > patches, and if you have further improvements over that submit them as
> > split up patches through the usual channels.  Having all architectures
> > actually build and work from mainline is really important to have
> > useful kernel package in distributions.
> 
> No, there has been no discussion of these patches, so there is no point in 
> doing this a few days before 2.6.13. Can we please do this properly for 
> 2.6.14?
>
> If you want to apply these patches, please also apply the following patch:

OK, fuck that.  Consider the patchbomb withdrawn.

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Roman Zippel]

Hi,

On Thu, 25 Aug 2005, Al Viro wrote:

> OK, fuck that.  Consider the patchbomb withdrawn.

Thanks.
Nobody is going to die that m68k doesn't compile again for another 
release. I appreciate the kick to get it going, but there is no point in 
forcing it a few days before the release, which basically leaves no other 
option than "merge now or die!".

bye, Roman

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Roman Zippel]

Hi,

On Thu, 25 Aug 2005, Al Viro wrote:

> On Thu, Aug 25, 2005 at 11:15:24AM +0200, Roman Zippel wrote:
> > >  
> > > -	*ti = *orig->thread_info;
> > >  	*tsk = *orig;
> > > +	setup_thread_info(tsk, ti);
> > >  	tsk->thread_info = ti;
> > >  	ti->task = tsk;
> > 
> > This introduces a subtle ordering requirement, where setup_thread_info 
> > magically finds in the new task_struct the pointer to the old thread_info 
> > to setup the new thread_info.
> 
> Nothing subtle with it, especially since this is the only place with any
> business to call setup_thread_info().

Wrong, we could have multiple versions setup_thread_info() and every one 
gets the pointer to old thread_info via the new task_struct.

> > What is your problem with what I have in CVS? There it completes the basic
> > task_struct setup and _after_ that it can setup the thread_info.
> 
> Which buys you what, exactly?  You end up with more things to do in
> setup_thread_info() and it doesn't get cleaner.

Wrong.

+static inline void setup_thread_stack(struct task_struct *p, struct task_struct *org)
+{
+       *task_thread_info(p) = *task_thread_info(org);
+       task_thread_info(p)->task = p;
+}

-       *ti = *orig->thread_info;
        *tsk = *orig;
        tsk->thread_info = ti;
-       ti->task = tsk;
+       setup_thread_stack(tsk, orig);

It's exactly the same work and setup_thread_stack() gets the old and new 
task_struct, with each one having the _correct_ thread_info.
If you really want to count cycles, the only minor difference is that 
some "ti" become "tsk->thread_info", but gcc is perfectly capable to 
detect that it's the same and it will generate the same code.

Moreover my code is cleaner, as it clearly separates two tasks:

	setup_task_struct(tsk, orig);
	setup_thread_stack(tsk, orig);


> > Al, I would really prefer to merge this one myself, I'm only waiting for 
> > the 2.6.13 release and since this is not a regression, I don't really 
> > understand why this must be in 2.6.13.
> 
> Fine, as long as that merge is done before your s/thread_info/stack/ patches.
> It should be the first step before doing 200Kb worth of cosmetical stuff
> that affects every architecture out there, not something that depends on
> it done.

Please count correctly, there is only one 100KB patch, the rest is rather 
small (50KB in 7 patches).

> There's also a question of having mainline build and work on the architecture
> in question, which obviously is not something you care about - this hairball
> had been sitting in m68k CVS for how long?  Since 2.5.60-something, with
> zero efforts to resolve it, right?  And mainline kernel didn't even build,
> let alone work since that moment.
> 
> FWIW, essentially the same splitup of that mess had been posted more than
> three months ago; definitely before 2.6.12-final.  Still no activity _and_
> plans that involve doing kernel-wide renaming of struct thread_info *
> thread_info in task_struct to void *stack as part of m68k merge.

Al, while I appreciate your iniative, could you please work a little bit 
more with the other people working on this port? I did take and adapted 
your patches and posted my versions of it and until yesterday you didn't 
bother to comment publically. The "no activity" is complete bullshit.

bye, Roman

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Al Viro]

On Thu, Aug 25, 2005 at 04:10:38PM +0200, Roman Zippel wrote:
> Hi,
> 
> On Thu, 25 Aug 2005, Al Viro wrote:
> 
> > On Thu, Aug 25, 2005 at 11:15:24AM +0200, Roman Zippel wrote:
> > > >  
> > > > -	*ti = *orig->thread_info;
> > > >  	*tsk = *orig;
> > > > +	setup_thread_info(tsk, ti);
> > > >  	tsk->thread_info = ti;
> > > >  	ti->task = tsk;
> > > 
> > > This introduces a subtle ordering requirement, where setup_thread_info 
> > > magically finds in the new task_struct the pointer to the old thread_info 
> > > to setup the new thread_info.
> > 
> > Nothing subtle with it, especially since this is the only place with any
> > business to call setup_thread_info().
> 
> Wrong, we could have multiple versions setup_thread_info() and every one 
> gets the pointer to old thread_info via the new task_struct.

How does that make what I wrote above wrong?  We do have two versions, all
right.  Called from one place.  Each is a one-liner in my variant, more than
that in your.
 
> > > What is your problem with what I have in CVS? There it completes the basic
> > > task_struct setup and _after_ that it can setup the thread_info.
> > 
> > Which buys you what, exactly?  You end up with more things to do in
> > setup_thread_info() and it doesn't get cleaner.
> 
> Wrong.
> 
> +static inline void setup_thread_stack(struct task_struct *p, struct task_struct *org)
> +{
> +       *task_thread_info(p) = *task_thread_info(org);
> +       task_thread_info(p)->task = p;
> +}

... and that does not fit "more things to do in setup_thread_info()" in
which way?

> > > Al, I would really prefer to merge this one myself, I'm only waiting for 
> > > the 2.6.13 release and since this is not a regression, I don't really 
> > > understand why this must be in 2.6.13.
> > 
> > Fine, as long as that merge is done before your s/thread_info/stack/ patches.
> > It should be the first step before doing 200Kb worth of cosmetical stuff
> > that affects every architecture out there, not something that depends on
> > it done.
> 
> Please count correctly, there is only one 100KB patch, the rest is rather 
> small (50KB in 7 patches).

... no comments, except that 28K (ti6_1) + 24K (ti6_2) + 22K (ti6_3) +
12K (ti6_4) already appears to be more than 50K...

In any case, that's hardly the point - s/200/150/ if you wish and that
does not make the problem much better.
 
> > There's also a question of having mainline build and work on the architecture
> > in question, which obviously is not something you care about - this hairball
> > had been sitting in m68k CVS for how long?  Since 2.5.60-something, with
> > zero efforts to resolve it, right?  And mainline kernel didn't even build,
> > let alone work since that moment.
> > 
> > FWIW, essentially the same splitup of that mess had been posted more than
> > three months ago; definitely before 2.6.12-final.  Still no activity _and_
> > plans that involve doing kernel-wide renaming of struct thread_info *
> > thread_info in task_struct to void *stack as part of m68k merge.
> 
> Al, while I appreciate your iniative, could you please work a little bit 
> more with the other people working on this port? I did take and adapted 
> your patches and posted my versions of it and until yesterday you didn't 
> bother to comment publically. The "no activity" is complete bullshit.

I seem to remember some very public conversations with you on that topic,
but again, this is not the point - the real issue is with merge strategy
you proposed.  And no, I do not believe that doing that merge + great
renaming in a single burst is feasible.  Reorder that and yes, all parts
make sense and are doable.  With essentially the same final tree.

Hell, I can even offer to do and feed the per-arch cleanups of that, getting
the final chunk down to tolerable size.  After and separate from getting m68k
to work in mainline.

To clarify the situation: this is _NOT_ an attempt to prevent ->thread_info
cleanups from getting into the tree.  I actually think that they do make
sense; moreover, they do make sense on their own and mixing them with m68k
merge only causes extra problems for both.

Re: [PATCH] (18/22) task_thread_info - part 2/4

[Roman Zippel]

Hi,

On Thu, 25 Aug 2005, Al Viro wrote:

> > > > >  
> > > > > -	*ti = *orig->thread_info;
> > > > >  	*tsk = *orig;
> > > > > +	setup_thread_info(tsk, ti);
> > > > >  	tsk->thread_info = ti;
> > > > >  	ti->task = tsk;
> > > > 
> > > > This introduces a subtle ordering requirement, where setup_thread_info 
> > > > magically finds in the new task_struct the pointer to the old thread_info 
> > > > to setup the new thread_info.
> 
> How does that make what I wrote above wrong?  We do have two versions, all
> right.  Called from one place.  Each is a one-liner in my variant, more than
> that in your.

The point is about ordering requirements that your "setup_thread_info(tsk, 
ti);" must be inbetween "*tsk = *orig;" and "tsk->thread_info = ti;", so 
that setup_thread_info() gets the right thread_info.

> > > > What is your problem with what I have in CVS? There it completes the basic
> > > > task_struct setup and _after_ that it can setup the thread_info.
> > > 
> > > Which buys you what, exactly?  You end up with more things to do in
> > > setup_thread_info() and it doesn't get cleaner.
> > 
> > Wrong.
> > 
> > +static inline void setup_thread_stack(struct task_struct *p, struct task_struct *org)
> > +{
> > +       *task_thread_info(p) = *task_thread_info(org);
> > +       task_thread_info(p)->task = p;
> > +}
> 
> ... and that does not fit "more things to do in setup_thread_info()" in
> which way?

That in the sum it's still the same work.

> > Please count correctly, there is only one 100KB patch, the rest is rather 
> > small (50KB in 7 patches).
> 
> ... no comments, except that 28K (ti6_1) + 24K (ti6_2) + 22K (ti6_3) +
> 12K (ti6_4) already appears to be more than 50K...

ti6 is the sum of ti6_?, to make it easier to verify.

> In any case, that's hardly the point - s/200/150/ if you wish and that
> does not make the problem much better.

Most of them rather simple search and replace.

> I seem to remember some very public conversations with you on that topic,
> but again, this is not the point

There has been a bit on IRC, but I can't really discuss such things on IRC 
(everyone just makes his own point and the issues continue and just scroll 
away) and it certainly didn't happen on the linux-m68k ml.

> - the real issue is with merge strategy
> you proposed.  And no, I do not believe that doing that merge + great
> renaming in a single burst is feasible.  Reorder that and yes, all parts
> make sense and are doable.  With essentially the same final tree.

The biggest part is already at the end and I'll split the big one into 
three separate patches (stack, end_of_stack and task_thread_info changes). 
I'll send them to Andrew and we can still discuss how quickly to merge 
each part. I don't really see the point in holding off for too long, 
except for the final thread_info field removal.

bye, Roman