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
-
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/