[PATCH] mm:Make the function zap_huge_pmd bool

[PATCH] mm:Make the function zap_huge_pmd bool

[Nicholas Krause]

This makes the function zap_huge_pmd have a return type of bool
now due to this particular function always returning one or zero
as its return value.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 include/linux/huge_mm.h | 2 +-
 mm/huge_memory.c        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index f10b20f..e83174e 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -19,7 +19,7 @@ extern struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
 					  unsigned long addr,
 					  pmd_t *pmd,
 					  unsigned int flags);
-extern int zap_huge_pmd(struct mmu_gather *tlb,
+extern bool zap_huge_pmd(struct mmu_gather *tlb,
 			struct vm_area_struct *vma,
 			pmd_t *pmd, unsigned long addr);
 extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index c107094..32b1993 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1384,11 +1384,11 @@ out:
 	return 0;
 }
 
-int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
+bool zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		 pmd_t *pmd, unsigned long addr)
 {
 	spinlock_t *ptl;
-	int ret = 0;
+	int ret = false;
 
 	if (__pmd_trans_huge_lock(pmd, vma, &ptl) == 1) {
 		struct page *page;
@@ -1419,7 +1419,7 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
 			tlb_remove_page(tlb, page);
 		}
 		pte_free(tlb->mm, pgtable);
-		ret = 1;
+		ret = true;
 	}
 	return ret;
 }
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Michal Hocko]

On Wed 01-07-15 14:27:57, Nicholas Krause wrote:
> This makes the function zap_huge_pmd have a return type of bool
> now due to this particular function always returning one or zero
> as its return value.

How does this help anything? IMO this just generates a pointless churn
in the code without a good reason.

> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  include/linux/huge_mm.h | 2 +-
>  mm/huge_memory.c        | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index f10b20f..e83174e 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -19,7 +19,7 @@ extern struct page *follow_trans_huge_pmd(struct vm_area_struct *vma,
>  					  unsigned long addr,
>  					  pmd_t *pmd,
>  					  unsigned int flags);
> -extern int zap_huge_pmd(struct mmu_gather *tlb,
> +extern bool zap_huge_pmd(struct mmu_gather *tlb,
>  			struct vm_area_struct *vma,
>  			pmd_t *pmd, unsigned long addr);
>  extern int mincore_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index c107094..32b1993 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1384,11 +1384,11 @@ out:
>  	return 0;
>  }
>  
> -int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> +bool zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  		 pmd_t *pmd, unsigned long addr)
>  {
>  	spinlock_t *ptl;
> -	int ret = 0;
> +	int ret = false;
>  
>  	if (__pmd_trans_huge_lock(pmd, vma, &ptl) == 1) {
>  		struct page *page;
> @@ -1419,7 +1419,7 @@ int zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  			tlb_remove_page(tlb, page);
>  		}
>  		pte_free(tlb->mm, pgtable);
> -		ret = 1;
> +		ret = true;
>  	}
>  	return ret;
>  }
> -- 
> 2.1.4
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Theodore Ts'o]

On Thu, Jul 02, 2015 at 09:26:21AM +0200, Michal Hocko wrote:
> On Wed 01-07-15 14:27:57, Nicholas Krause wrote:
> > This makes the function zap_huge_pmd have a return type of bool
> > now due to this particular function always returning one or zero
> > as its return value.
> 
> How does this help anything? IMO this just generates a pointless churn
> in the code without a good reason.

Hi Michal,

My recommendation is to ignore patches sent by Nick.  In my experience
he doesn't understand code before trying to make mechanical changes,
and very few of his patches add any new value, and at least one that
he tried to send me just 2 weeks or so ago (cherry-picked to try to
"prove" why he had turned over a new leaf, so that I would support the
removal of his e-mail address from being blacklisted on
vger.kernel.org) was buggy, and when I asked him some basic questions
about what the code was doing, it was clear he had no clue how the
seq_file abstraction worked.  This didn't stop him from trying to
patch the code, and if he had tested it, it would have crashed and
burned instantly.

Of course, do whatevery you want, but IMHO it's not really not worth
your time to deal with his patches, and if you reply, most people
won't see his original e-mail since the vger.kernel.org blacklist is
still in effect.

Regards,

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-02 12:03 PM, Theodore Ts'o wrote:
> On Thu, Jul 02, 2015 at 09:26:21AM +0200, Michal Hocko wrote:
>> On Wed 01-07-15 14:27:57, Nicholas Krause wrote:
>>> This makes the function zap_huge_pmd have a return type of bool
>>> now due to this particular function always returning one or zero
>>> as its return value.
>>
>> How does this help anything? IMO this just generates a pointless churn
>> in the code without a good reason.
> 
> Hi Michal,
> 
> My recommendation is to ignore patches sent by Nick.  In my experience
> he doesn't understand code before trying to make mechanical changes,
> and very few of his patches add any new value, and at least one that
> he tried to send me just 2 weeks or so ago (cherry-picked to try to
> "prove" why he had turned over a new leaf, so that I would support the
> removal of his e-mail address from being blacklisted on
> vger.kernel.org) was buggy, and when I asked him some basic questions
> about what the code was doing, it was clear he had no clue how the
> seq_file abstraction worked.  This didn't stop him from trying to
> patch the code, and if he had tested it, it would have crashed and
> burned instantly.
> 
> Of course, do whatevery you want, but IMHO it's not really not worth
> your time to deal with his patches, and if you reply, most people
> won't see his original e-mail since the vger.kernel.org blacklist is
> still in effect.
> 
> Regards,
> 
> 						- Ted
> 
Ted,
I looked into that patch further and would were correct it was wrong.
However here is a bug fix for the drm driver code that somebody else
stated was right but haven gotten a reply to from the maintainer and
have tried resending.
Nick

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-02 12:08 PM, nick wrote:
> 
> 
> On 2015-07-02 12:03 PM, Theodore Ts'o wrote:
>> On Thu, Jul 02, 2015 at 09:26:21AM +0200, Michal Hocko wrote:
>>> On Wed 01-07-15 14:27:57, Nicholas Krause wrote:
>>>> This makes the function zap_huge_pmd have a return type of bool
>>>> now due to this particular function always returning one or zero
>>>> as its return value.
>>>
>>> How does this help anything? IMO this just generates a pointless churn
>>> in the code without a good reason.
>>
>> Hi Michal,
>>
>> My recommendation is to ignore patches sent by Nick.  In my experience
>> he doesn't understand code before trying to make mechanical changes,
>> and very few of his patches add any new value, and at least one that
>> he tried to send me just 2 weeks or so ago (cherry-picked to try to
>> "prove" why he had turned over a new leaf, so that I would support the
>> removal of his e-mail address from being blacklisted on
>> vger.kernel.org) was buggy, and when I asked him some basic questions
>> about what the code was doing, it was clear he had no clue how the
>> seq_file abstraction worked.  This didn't stop him from trying to
>> patch the code, and if he had tested it, it would have crashed and
>> burned instantly.
>>
>> Of course, do whatevery you want, but IMHO it's not really not worth
>> your time to deal with his patches, and if you reply, most people
>> won't see his original e-mail since the vger.kernel.org blacklist is
>> still in effect.
>>
>> Regards,
>>
>> 						- Ted
>>
> Ted,
> I looked into that patch further and would were correct it was wrong.
> However here is a bug fix for the drm driver code that somebody else
> stated was right but haven gotten a reply to from the maintainer and
> have tried resending.
> Nick
> From 2d2ddb5d9a2c4fcbae45339d4f775fcde49f36e1 Mon Sep 17 00:00:00 2001
> From: Nicholas Krause <xerofoify@gmail.com>
> Date: Wed, 13 May 2015 21:36:44 -0400
> Subject: [PATCH 1/2] gma500:Add proper use of the variable ret for the
>  function, psb_mmu_inset_pfn_sequence
> 
> This adds proper use of the variable ret by returning it
> at the end of the function, psb_mmu_inset_pfn_sequence for
> indicating to callers when an error has occurred. Further
> more remove the unneeded double setting of ret to the error
> code, -ENOMEM after checking if a call to the function,
> psb_mmu_pt_alloc_map_lock is successful.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/gpu/drm/gma500/mmu.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
> index 0eaf11c..d2c4bac 100644
> --- a/drivers/gpu/drm/gma500/mmu.c
> +++ b/drivers/gpu/drm/gma500/mmu.c
> @@ -677,10 +677,9 @@ int psb_mmu_insert_pfn_sequence(struct psb_mmu_pd *pd, uint32_t start_pfn,
>  	do {
>  		next = psb_pd_addr_end(addr, end);
>  		pt = psb_mmu_pt_alloc_map_lock(pd, addr);
> -		if (!pt) {
> -			ret = -ENOMEM;
> +		if (!pt)
>  			goto out;
> -		}
> +
>  		do {
>  			pte = psb_mmu_mask_pte(start_pfn++, type);
>  			psb_mmu_set_pte(pt, addr, pte);
> @@ -700,7 +699,7 @@ out:
>  	if (pd->hw_context != -1)
>  		psb_mmu_flush(pd->driver);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  int psb_mmu_insert_pages(struct psb_mmu_pd *pd, struct page **pages,
> 
Ted,
Here are some other patches from this week and before that may be more useful then the ones applied/
acknowledged.
Nick

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Theodore Ts'o]

On Thu, Jul 02, 2015 at 12:08:36PM -0400, nick wrote:
> I looked into that patch further and would were correct it was wrong.
> However here is a bug fix for the drm driver code that somebody else
> stated was right but haven gotten a reply to from the maintainer and
> have tried resending.

Hi Nick,

Don't bother sending more low-value patches like this; they don't
impress me.  Send me a patch that fixes a deep bug, where you can
demonstrate that you understand the underlying design of the code, can
point out a flaw, and then explain why your patch is an improvement,
and documents how you tested it.  Or do something beyond changing
return values or return types, and optimize some performance-critical
part of the kernel, and in the commit description, explain why it
improves things, how you measured the performance improvement, and why
this is applicable in a real-life situation.

Even a broken clock can be right twice a day, and the fact that it is
possible that you can author a correct patch isn't all that
impressive.  You need to understand deep understanding of the code you
are modifying, and or else it's not worth my time to go through a
large number of low-value patches that don't really improve the code
base much, when the risk that you have accidentally introduced a bug
is high given that (a) you've demonstrated an inability to explain
some of your patches, and (b) in many cases, you have no fear about
sending patches that you can't personally test.  These two
shortcomings in combination are fatal.

If you can demonstrate that you can become a thoughtful and careful
coder, I would be most pleased to argue to Greg K-H that you have
turned over a new leaf.  To date, however, you have not demonstrated
any of the above, and you've made me regret that I've tried to waste
time looking at your patches that you've sent me in the hopes of
convincing me that you've really changed --- when it's clear you
haven't.  I do hope that, one day, you will be able to be a good
coder.  But that day is clearly not today.

Best regards,

					- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-03 10:46 AM, Theodore Ts'o wrote:
> On Thu, Jul 02, 2015 at 12:08:36PM -0400, nick wrote:
>> I looked into that patch further and would were correct it was wrong.
>> However here is a bug fix for the drm driver code that somebody else
>> stated was right but haven gotten a reply to from the maintainer and
>> have tried resending.
> 
> Hi Nick,
> 
> Don't bother sending more low-value patches like this; they don't
> impress me.  Send me a patch that fixes a deep bug, where you can
> demonstrate that you understand the underlying design of the code, can
> point out a flaw, and then explain why your patch is an improvement,
> and documents how you tested it.  Or do something beyond changing
> return values or return types, and optimize some performance-critical
> part of the kernel, and in the commit description, explain why it
> improves things, how you measured the performance improvement, and why
> this is applicable in a real-life situation.
> 
> Even a broken clock can be right twice a day, and the fact that it is
> possible that you can author a correct patch isn't all that
> impressive.  You need to understand deep understanding of the code you
> are modifying, and or else it's not worth my time to go through a
> large number of low-value patches that don't really improve the code
> base much, when the risk that you have accidentally introduced a bug
> is high given that (a) you've demonstrated an inability to explain
> some of your patches, and (b) in many cases, you have no fear about
> sending patches that you can't personally test.  These two
> shortcomings in combination are fatal.
> 
> If you can demonstrate that you can become a thoughtful and careful
> coder, I would be most pleased to argue to Greg K-H that you have
> turned over a new leaf.  To date, however, you have not demonstrated
> any of the above, and you've made me regret that I've tried to waste
> time looking at your patches that you've sent me in the hopes of
> convincing me that you've really changed --- when it's clear you
> haven't.  I do hope that, one day, you will be able to be a good
> coder.  But that day is clearly not today.
> 
> Best regards,
> 
> 					- Ted
> 
Did you even look at the other patches I send you. Here is a bug fix for the gma500 driver code
that someone else stated is right but I don't have the hardware so it's difficult to test.
Nick

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Michal Hocko]

On Fri 03-07-15 10:54:07, nick wrote:
[...]
> Did you even look at the other patches I send you. Here is a bug fix
> for the gma500 driver code that someone else stated is right but I
> don't have the hardware so it's difficult to test.

This is really annoying. Please stop it! Ted is not maintainer of the
code you are trying to patch. There is absolutely no reason to try to
persuate him or try to push it through him. Go and try to "sell" your
patch to the maintainers of the said code.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-03 11:01 AM, Michal Hocko wrote:
> On Fri 03-07-15 10:54:07, nick wrote:
> [...]
>> Did you even look at the other patches I send you. Here is a bug fix
>> for the gma500 driver code that someone else stated is right but I
>> don't have the hardware so it's difficult to test.
> 
> This is really annoying. Please stop it! Ted is not maintainer of the
> code you are trying to patch. There is absolutely no reason to try to
> persuate him or try to push it through him. Go and try to "sell" your
> patch to the maintainers of the said code.
> 
Michael,
The reason I am doing this is Ted is trying to find a bug that I fixed in order to prove
to Greg Kroah Hartman I have changed. Otherwise I would be pushing this through the
drm maintainer(s).
Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Theodore Ts'o]

On Fri, Jul 03, 2015 at 11:03:11AM -0400, nick wrote:
> 
> The reason I am doing this is Ted is trying to find a bug that I
> fixed in order to prove to Greg Kroah Hartman I have
> changed. Otherwise I would be pushing this through the drm
> maintainer(s).

I am trying to determine if you have changed.  Your comment justifying
your lack of testing because "it's hard to test" is ample evidence
that you have *not* changed.

Simply coming up with a commit that happens to be correct is a
necessary, but not sufficient condition.  Especially when you feel
that you need to send dozens of low-value patches and hope that one of
them is correct, and then use that as "proof".  It's the attitude
which is problem, not whether or not you can manage to come up with a
correct patch.

I've described to you what you need to do in order to demonstrate that
you have the attitude and inclinations in order to be a kernel
developer that a maintainer can trust as being capable of authoring a
patch that doesn't create more problems than whatever benefits it
might have.  I respectfully ask that you try to work on that, and stop
bothering me (and everyone else).

Best regards,

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-03 12:49 PM, Theodore Ts'o wrote:
> On Fri, Jul 03, 2015 at 11:03:11AM -0400, nick wrote:
>>
>> The reason I am doing this is Ted is trying to find a bug that I
>> fixed in order to prove to Greg Kroah Hartman I have
>> changed. Otherwise I would be pushing this through the drm
>> maintainer(s).
> 
> I am trying to determine if you have changed.  Your comment justifying
> your lack of testing because "it's hard to test" is ample evidence
> that you have *not* changed.
> 
> Simply coming up with a commit that happens to be correct is a
> necessary, but not sufficient condition.  Especially when you feel
> that you need to send dozens of low-value patches and hope that one of
> them is correct, and then use that as "proof".  It's the attitude
> which is problem, not whether or not you can manage to come up with a
> correct patch.
> 
> I've described to you what you need to do in order to demonstrate that
> you have the attitude and inclinations in order to be a kernel
> developer that a maintainer can trust as being capable of authoring a
> patch that doesn't create more problems than whatever benefits it
> might have.  I respectfully ask that you try to work on that, and stop
> bothering me (and everyone else).
> 
> Best regards,
> 
> 						- Ted
> 
Ted,
I agree with you 100 percent. The reason I can't test this is I don't have the
hardware otherwise I would have tested it by now.
Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Theodore Ts'o]

On Fri, Jul 03, 2015 at 12:52:06PM -0400, nick wrote:
> I agree with you 100 percent. The reason I can't test this is I don't have the
> hardware otherwise I would have tested it by now.

Then don't send the patch out.  Work on some other piece of part of
the kernel, or better yet, some other userspace code where testing is
easier.  It's really quite simple.

You don't have the technical skills, or at this point, the reputation,
to send patches without tesitng them first.  The fact that sometimes
people like Linus will send out a patch labelled with "COMPLETELY
UNTESTED", is because he's skilled and trusted enough that he can get
away with it.  You have neither of those advantages.

Best regards,

						- Ted

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-03 02:45 PM, Theodore Ts'o wrote:
> On Fri, Jul 03, 2015 at 12:52:06PM -0400, nick wrote:
>> I agree with you 100 percent. The reason I can't test this is I don't have the
>> hardware otherwise I would have tested it by now.
> 
> Then don't send the patch out.  Work on some other piece of part of
> the kernel, or better yet, some other userspace code where testing is
> easier.  It's really quite simple.
> 
> You don't have the technical skills, or at this point, the reputation,
> to send patches without tesitng them first.  The fact that sometimes
> people like Linus will send out a patch labelled with "COMPLETELY
> UNTESTED", is because he's skilled and trusted enough that he can get
> away with it.  You have neither of those advantages.
> 
> Best regards,
> 
> 						- Ted
> 
Ted,
My system breaks due to the commit id 7202ab46f7392265c1ecbf03f600393bf32a8bdf on boot
as I get a hang and black screen. In addition I even attempted single user mode but this
still happens with this commit. I found this by running git bisect on my system for the 
last few hours and hope this is more useful them my trivial cleanup patches. 
Nick 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[Mel Gorman]

On Fri, Jul 03, 2015 at 02:45:02PM -0400, Theodore Ts'o wrote:
> On Fri, Jul 03, 2015 at 12:52:06PM -0400, nick wrote:
> > I agree with you 100 percent. The reason I can't test this is I don't have the
> > hardware otherwise I would have tested it by now.
> 
> Then don't send the patch out.  Work on some other piece of part of
> the kernel, or better yet, some other userspace code where testing is
> easier.  It's really quite simple.
> 
> You don't have the technical skills, or at this point, the reputation,
> to send patches without tesitng them first.  The fact that sometimes
> people like Linus will send out a patch labelled with "COMPLETELY
> UNTESTED", is because he's skilled and trusted enough that he can get
> away with it.

It's not just that. In all cases I'm aware of, Linus was illustrating his
point by means of a patch during a discussion. It was expected that the
developer or user that started the discussion would take that patch and run
with it if it was heading in the correct direction. In exceptional cases,
the patch would be merged after a confirmation from that developer or user
that the patch worked for whatever problem they faced. The only time I've
seen a COMPLETELY UNTESTED patch merged was when it was painfully obvious it
was correct and more importantly, it solved a specific problem. Linus is not
the only developer that does this style of discussion through untested patch.

In other cases where an untested patch has been merged, it was either due to
it being exceptionally trivial or a major API change that affects a number
of subsystems (like adding a new filesystem API for example). In the former
case, it's usually self-evident and often tacked onto a larger series where
there is a degree of trust. In the latter case, all cases they can test
have been covered and the code for the remaining hardware was altered in
a very similar manner. This also lends some confidence that the transform
is ok because similar transforms were tested and known to be correct.

For trivial patches that alter just return values there are a few hazards. A
mistake can be made introducing a real bug with the upside being marginal or
non-existent. That's a very poor tradeoff and generally why checkpatch-only
patches fall by the wayside. Readability is a two-edged sword. Maybe the
code is marginally easier to read but it's sometimes offset by the fact
that git blame no longer points to the important origin of the code. If
a real bug is being investigated then all the cleanup patches have to be
identified and dismissed which consumes time and concentration.

Cleanups in my opinion are ok in two cases. The first is if it genuinely
makes the code much easier to follow. In cases where I've seen this, it was
done because the code was either unreadable or it was in preparation for a
more relevant patch series that was then easier to review and justified the
cleanup. The second is where the affected code is being heavily modified
anyway so the cleanup while you are there is both useful and does not
impact git blame.

This particular patch does not match any of the criteria. The DRM patch
may or may not be correct but there is no way I'd expect something like
it to be picked up without testing or in reference to a bug report.

For this patch, NAK. Nick, from me at least consider any similar patch
affecting mm/ that modifies return values or types without being part of
a larger series that addresses a particular problem to be silently NAKed
or filed under "doesn't matter" by me.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function zap_huge_pmd bool

[nick]

On 2015-07-03 04:13 PM, Mel Gorman wrote:
> On Fri, Jul 03, 2015 at 02:45:02PM -0400, Theodore Ts'o wrote:
>> On Fri, Jul 03, 2015 at 12:52:06PM -0400, nick wrote:
>>> I agree with you 100 percent. The reason I can't test this is I don't have the
>>> hardware otherwise I would have tested it by now.
>>
>> Then don't send the patch out.  Work on some other piece of part of
>> the kernel, or better yet, some other userspace code where testing is
>> easier.  It's really quite simple.
>>
>> You don't have the technical skills, or at this point, the reputation,
>> to send patches without tesitng them first.  The fact that sometimes
>> people like Linus will send out a patch labelled with "COMPLETELY
>> UNTESTED", is because he's skilled and trusted enough that he can get
>> away with it.
> 
> It's not just that. In all cases I'm aware of, Linus was illustrating his
> point by means of a patch during a discussion. It was expected that the
> developer or user that started the discussion would take that patch and run
> with it if it was heading in the correct direction. In exceptional cases,
> the patch would be merged after a confirmation from that developer or user
> that the patch worked for whatever problem they faced. The only time I've
> seen a COMPLETELY UNTESTED patch merged was when it was painfully obvious it
> was correct and more importantly, it solved a specific problem. Linus is not
> the only developer that does this style of discussion through untested patch.
> 
> In other cases where an untested patch has been merged, it was either due to
> it being exceptionally trivial or a major API change that affects a number
> of subsystems (like adding a new filesystem API for example). In the former
> case, it's usually self-evident and often tacked onto a larger series where
> there is a degree of trust. In the latter case, all cases they can test
> have been covered and the code for the remaining hardware was altered in
> a very similar manner. This also lends some confidence that the transform
> is ok because similar transforms were tested and known to be correct.
> 
> For trivial patches that alter just return values there are a few hazards. A
> mistake can be made introducing a real bug with the upside being marginal or
> non-existent. That's a very poor tradeoff and generally why checkpatch-only
> patches fall by the wayside. Readability is a two-edged sword. Maybe the
> code is marginally easier to read but it's sometimes offset by the fact
> that git blame no longer points to the important origin of the code. If
> a real bug is being investigated then all the cleanup patches have to be
> identified and dismissed which consumes time and concentration.
> 
> Cleanups in my opinion are ok in two cases. The first is if it genuinely
> makes the code much easier to follow. In cases where I've seen this, it was
> done because the code was either unreadable or it was in preparation for a
> more relevant patch series that was then easier to review and justified the
> cleanup. The second is where the affected code is being heavily modified
> anyway so the cleanup while you are there is both useful and does not
> impact git blame.
> 
> This particular patch does not match any of the criteria. The DRM patch
> may or may not be correct but there is no way I'd expect something like
> it to be picked up without testing or in reference to a bug report.
> 
> For this patch, NAK. Nick, from me at least consider any similar patch
> affecting mm/ that modifies return values or types without being part of
> a larger series that addresses a particular problem to be silently NAKed
> or filed under "doesn't matter" by me.
> 
I am no longer going to do cleanups as they are a waste of mine and the maintainers,
thanks for pointing that out through Mel. However I did find a bad commit with the id 7202ab46f7392265c1ecbf03f600393bf32a8bdf that breaks and hangs my system on boot so
bad I can't even get into single user mode. Either one of the maintainers of that
code can revert it or we can find a proper fix to it on my broken machine with this
commit. In addition all of my patches for cleanups that don't remove unused
code have been removed, there of no use. However this patch has been on my system
for a month and is an important cleanup as it removes other 100 lines of unused
code. I also have another function removal patch for both chelsio drivers and
the mailbox api that are staying around due to them being useful in terms of 
removing old/unused code. 
Thanks A lot for Your and Ted's Time,
Nick