Messages in this thread | | | Date | Sat, 23 Feb 2008 22:08:52 -0500 | From | Christoph Hellwig <> | Subject | Re: [patch 2/2] x86,fpu: lazy allocation of FPU area |
| |
On Sat, Feb 23, 2008 at 06:34:39PM -0800, Suresh Siddha wrote: > + if (!tsk->thread.cntxt) > + tsk->thread.cntxt = alloc_cntxt_struct();
Please use tabs, not spaces for indentation.
> +union thread_cntxt *alloc_cntxt_struct(void) > +{ > + return kmem_cache_alloc(task_cntxt_cachep, GFP_KERNEL); > +}
Why do you need this wrapper at all?
> +struct task_struct * alloc_task_struct(struct task_struct *src) > { > struct task_struct *tsk; > tsk = kmem_cache_alloc(task_struct_cachep, GFP_KERNEL); > + if (tsk && src->thread.cntxt) { > + tsk->thread.cntxt = alloc_cntxt_struct(); > + if (!tsk->thread.cntxt) { > + kmem_cache_free(task_struct_cachep, tsk); > + return NULL; > + } > + WARN_ON((unsigned long)tsk->thread.cntxt & 15); > + } else if (tsk) > + tsk->thread.cntxt = NULL; > return tsk;
I think passing the src here doesn't make much sense. Copying the fpu context structure should be done by the arch dup_task_struct hook. Now if you free the fpu context in free_thread_info that means you don't have to have your own arch-specific task_struct allocator which would be a _lot_ cleaner. Talking about clean I think the .cntxt field really wants a more descriptive name :)
| |