[PATCH] futex: use wake_up_process() instead of wake_up_state()

[PATCH] futex: use wake_up_process() instead of wake_up_state()

[Wang Qing]

Using wake_up_process() is more simpler and friendly, 
and it is more convenient for analysis and statistics

Signed-off-by: Wang Qing <wangqing@vivo.com>
---
 kernel/futex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index e68db77..078a1f9
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
 
 	q->lock_ptr = &hb->lock;
 
-	wake_up_state(q->task, TASK_NORMAL);
+	wake_up_process(q->task);
 }
 
 /**
-- 
2.7.4

Re: [PATCH] futex: use wake_up_process() instead of wake_up_state()

[Mike Galbraith]

On Fri, 2021-03-19 at 10:59 +0800, Wang Qing wrote:
> Using wake_up_process() is more simpler and friendly,
> and it is more convenient for analysis and statistics

I likely needn't bother, and don't have a NAK to paste on this thing,
but here's another copy of my NOPE for yet another gratuitous change
with complete BS justification.

>
> Signed-off-by: Wang Qing <wangqing@vivo.com>
> ---
>  kernel/futex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index e68db77..078a1f9
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
>
>  	q->lock_ptr = &hb->lock;
>
> -	wake_up_state(q->task, TASK_NORMAL);
> +	wake_up_process(q->task);
>  }
>
>  /**

Re: [PATCH] futex: use wake_up_process() instead of wake_up_state()

[Mike Galbraith]

On Fri, 2021-03-19 at 06:21 +0100, Mike Galbraith wrote:
> On Fri, 2021-03-19 at 10:59 +0800, Wang Qing wrote:
> > Using wake_up_process() is more simpler and friendly,
> > and it is more convenient for analysis and statistics
>
> I likely needn't bother, and don't have a NAK to paste on this thing,
> but here's another copy of my NOPE for yet another gratuitous change
> with complete BS justification.

Let me try a bit softer tone.  I think you're trying to help, but
ignoring feedback is not the way to achieve that goal.  My feedback was
and remains that your change is not an improvement, it's churn, but
more importantly, that changes require technical justification, which
you did not provide.  You were subsequently handed the justification
you lacked by none other than the maintainer of the code you were
modifying.  He told you that your change could become a tiny kernel
size optimization by converting like instances all in one patch.. which
you promptly ignored, instead submitting multiple patches with zero
justification.  That is not the path to success.

>
> >
> > Signed-off-by: Wang Qing <wangqing@vivo.com>
> > ---
> >  kernel/futex.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/futex.c b/kernel/futex.c
> > index e68db77..078a1f9
> > --- a/kernel/futex.c
> > +++ b/kernel/futex.c
> > @@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
> >
> >  	q->lock_ptr = &hb->lock;
> >
> > -	wake_up_state(q->task, TASK_NORMAL);
> > +	wake_up_process(q->task);
> >  }
> >
> >  /**
>

Re:Re: [PATCH] futex: use wake_up_process() instead of wake_up_state()

[王擎]

>> On Fri, 2021-03-19 at 10:59 +0800, Wang Qing wrote:
>> > Using wake_up_process() is more simpler and friendly,
>> > and it is more convenient for analysis and statistics
>>
>> I likely needn't bother, and don't have a NAK to paste on this thing,
>> but here's another copy of my NOPE for yet another gratuitous change
>> with complete BS justification.
>
>Let me try a bit softer tone.  I think you're trying to help, but
>ignoring feedback is not the way to achieve that goal.  My feedback was
>and remains that your change is not an improvement, it's churn, but
>more importantly, that changes require technical justification, which
>you did not provide.  You were subsequently handed the justification
>you lacked by none other than the maintainer of the code you were
>modifying.  He told you that your change could become a tiny kernel
>size optimization by converting like instances all in one patch.. which
>you promptly ignored, instead submitting multiple patches with zero
>justification.  That is not the path to success.

Thank you for your reply. There are two reasons for sending patch again. 
One is that I think this is only an improvement in format and has no 
substantial impact, so no verification is required. 
The second one is that I want to hear more opinions from the maintainer. 
Because the entire kernel may have similar problems, I have to figure out 
whether this is a tacit behavior.
Also, I don't understand what you mean by "your change could become a 
tiny kernel size optimization by converting like instances all in one patch".

Thanks,
WangQing.

>
>>
>> >
>> > Signed-off-by: Wang Qing <wangqing@vivo.com>
>> > ---
>> >  kernel/futex.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/kernel/futex.c b/kernel/futex.c
>> > index e68db77..078a1f9
>> > --- a/kernel/futex.c
>> > +++ b/kernel/futex.c
>> > @@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
>> >
>> >  	q->lock_ptr = &hb->lock;
>> >
>> > -	wake_up_state(q->task, TASK_NORMAL);
>> > +	wake_up_process(q->task);
>> >  }
>> >
>> >  /**
>>
>

Re: Re: [PATCH] futex: use wake_up_process() instead of wake_up_state()

[Mike Galbraith]

On Fri, 2021-03-19 at 17:15 +0800, 王擎 wrote:
> >> On Fri, 2021-03-19 at 10:59 +0800, Wang Qing wrote:
> >> > Using wake_up_process() is more simpler and friendly,
> >> > and it is more convenient for analysis and statistics
> >>
> >> I likely needn't bother, and don't have a NAK to paste on this thing,
> >> but here's another copy of my NOPE for yet another gratuitous change
> >> with complete BS justification.
> >
> >Let me try a bit softer tone.  I think you're trying to help, but
> >ignoring feedback is not the way to achieve that goal.  My feedback was
> >and remains that your change is not an improvement, it's churn, but
> >more importantly, that changes require technical justification, which
> >you did not provide.  You were subsequently handed the justification
> >you lacked by none other than the maintainer of the code you were
> >modifying.  He told you that your change could become a tiny kernel
> >size optimization by converting like instances all in one patch.. which
> >you promptly ignored, instead submitting multiple patches with zero
> >justification.  That is not the path to success.
>
> Thank you for your reply. There are two reasons for sending patch again.
> One is that I think this is only an improvement in format and has no
> substantial impact, so no verification is required.

No substantial impact is not an argument in favor, quite the contrary.

> The second one is that I want to hear more opinions from the maintainer.

The highly qualified opinion Ingo took the time to hand you on a silver
platter was not good enough?  I'm starting to regret being concerned
about having potentially discouraged a potential contributor.

> Because the entire kernel may have similar problems, I have to figure out
> whether this is a tacit behavior.

My obviously misguided concerns just appeared in my rear view mirror.

> Also, I don't understand what you mean by "your change could become a
> tiny kernel size optimization by converting like instances all in one patch".

Go back and read what Ingo wrote.  While at it, read what the other two
maintainers who chimed in had to say before you consider doing any re-
submission.  The low hanging fruit you're trying to pick isn't anywhere
near as juicy as you seem to think it is.

> >>
> >> >
> >> > Signed-off-by: Wang Qing <wangqing@vivo.com>
> >> > ---
> >> >  kernel/futex.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/kernel/futex.c b/kernel/futex.c
> >> > index e68db77..078a1f9
> >> > --- a/kernel/futex.c
> >> > +++ b/kernel/futex.c
> >> > @@ -1820,7 +1820,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
> >> >
> >> >  	q->lock_ptr = &hb->lock;
> >> >
> >> > -	wake_up_state(q->task, TASK_NORMAL);
> >> > +	wake_up_process(q->task);
> >> >  }
> >> >
> >> >  /**
> >>
> >
>
>