[patch] x86: Unneeded assignment to tsk in recent x86 change

[patch] x86: Unneeded assignment to tsk in recent x86 change

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

Hello!

Impact: Cleanup.

Remove unneeded assignment to tsk in recent x86 change [1].

[1]: http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=d737c7649e2f7bdaa8760a9205dffaa45c117f20

Signed-off-by: Uros Bizjak <ubizjak@gmail.com>

Patch vs. tip/master.

Uros.

[-- Attachment #2: l.diff.txt --]
[-- Type: text/plain, Size: 795 bytes --]

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 93a563b..621e9b3 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -421,7 +421,6 @@ static noinline void pgtable_bad(struct pt_regs *regs,
 	printk(KERN_ALERT "%s: Corrupted page table at address %lx\n",
 	       tsk->comm, address);
 	dump_pagetable(address);
-	tsk = current;
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
@@ -795,13 +794,12 @@ asmlinkage
 void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
 	unsigned long address;
-	struct task_struct *tsk;
+	struct task_struct *tsk = current;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
 	int write;
 	int fault;
 
-	tsk = current;
 	mm = tsk->mm;
 	prefetchw(&mm->mmap_sem);
 

Re: [patch] x86: Unneeded assignment to tsk in recent x86 change

[Mikael Pettersson]

Uros Bizjak writes:
 > Hello!
 > 
 > Impact: Cleanup.
 > 
 > Remove unneeded assignment to tsk in recent x86 change [1].
 > 
 > [1]: http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-x86.git;a=commitdiff;h=d737c7649e2f7bdaa8760a9205dffaa45c117f20
 > 
 > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
 > 
 > Patch vs. tip/master.
 > 
 > Uros.
 > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
 > index 93a563b..621e9b3 100644
 > --- a/arch/x86/mm/fault.c
 > +++ b/arch/x86/mm/fault.c
 > @@ -421,7 +421,6 @@ static noinline void pgtable_bad(struct pt_regs *regs,
 >  	printk(KERN_ALERT "%s: Corrupted page table at address %lx\n",
 >  	       tsk->comm, address);
 >  	dump_pagetable(address);
 > -	tsk = current;
 >  	tsk->thread.cr2 = address;
 >  	tsk->thread.trap_no = 14;
 >  	tsk->thread.error_code = error_code;

this bit is ok, clearly *tsk is valid and == current before the assignment

 > @@ -795,13 +794,12 @@ asmlinkage
 >  void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 >  {
 >  	unsigned long address;
 > -	struct task_struct *tsk;
 > +	struct task_struct *tsk = current;
 >  	struct mm_struct *mm;
 >  	struct vm_area_struct *vma;
 >  	int write;
 >  	int fault;
 >  
 > -	tsk = current;
 >  	mm = tsk->mm;
 >  	prefetchw(&mm->mmap_sem);

but this is neither a fix nor IMO a cleanup (it's inconsistent with
the other variables in that function)

Re: [patch] x86: Unneeded assignment to tsk in recent x86 change

[Uros Bizjak]

Mikael Pettersson wrote:

>  > Impact: Cleanup.
>  > 
>  > Remove unneeded assignment to tsk in recent x86 change [1].
>  
>
>  > @@ -795,13 +794,12 @@ asmlinkage
>  >  void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
>  >  {
>  >  	unsigned long address;
>  > -	struct task_struct *tsk;
>  > +	struct task_struct *tsk = current;
>  >  	struct mm_struct *mm;
>  >  	struct vm_area_struct *vma;
>  >  	int write;
>  >  	int fault;
>  >  
>  > -	tsk = current;
>  >  	mm = tsk->mm;
>  >  	prefetchw(&mm->mmap_sem);
>
> but this is neither a fix nor IMO a cleanup (it's inconsistent with
> the other variables in that function)
>   

Hm, I'm not sure I see the inconsistency here. Care to explain this 
inconsistency in a couple of words?

Thanks,
Uros.

Re: [patch] x86: Unneeded assignment to tsk in recent x86 change

[Mikael Pettersson]

Uros Bizjak writes:
 > Mikael Pettersson wrote:
 > 
 > >  > Impact: Cleanup.
 > >  > 
 > >  > Remove unneeded assignment to tsk in recent x86 change [1].
 > >  
 > >
 > >  > @@ -795,13 +794,12 @@ asmlinkage
 > >  >  void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 > >  >  {
 > >  >  	unsigned long address;
 > >  > -	struct task_struct *tsk;
 > >  > +	struct task_struct *tsk = current;
 > >  >  	struct mm_struct *mm;
 > >  >  	struct vm_area_struct *vma;
 > >  >  	int write;
 > >  >  	int fault;
 > >  >  
 > >  > -	tsk = current;
 > >  >  	mm = tsk->mm;
 > >  >  	prefetchw(&mm->mmap_sem);
 > >
 > > but this is neither a fix nor IMO a cleanup (it's inconsistent with
 > > the other variables in that function)
 > >   
 > 
 > Hm, I'm not sure I see the inconsistency here. Care to explain this 
 > inconsistency in a couple of words?

You're initialising some variables in their declarations, and some
using assignments at the start of the procedure body. In particular,
for some reason you don't initialise 'mm' in its declaration even
though you could do so for consistency with 'tsk'.

However, I'm strongly in favour of separating declarations and
initialisations (esp. ones that need actual computations), so
rather than subjecting 'mm' to the treatment you gave 'tsk',
just leave the code alone.