* [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK @ 2019-07-23 3:30 Yang Xu 2019-07-23 7:23 ` Cyrill Gorcunov 0 siblings, 1 reply; 9+ messages in thread From: Yang Xu @ 2019-07-23 3:30 UTC (permalink / raw) To: akpm, gorcunov; +Cc: linux-kernel, Yang Xu arg2 will never < 0, for its type is 'unsigned long'. So negative judgment is meaningless. Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- kernel/sys.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..399457d26bef 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: - if (arg2 <= 0) + if (arg2) + current->timer_slack_ns = arg2; + else current->timer_slack_ns = current->default_timer_slack_ns; - else - current->timer_slack_ns = arg2; break; case PR_MCE_KILL: if (arg4 | arg5) -- 2.18.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK 2019-07-23 3:30 [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK Yang Xu @ 2019-07-23 7:23 ` Cyrill Gorcunov 2019-07-23 8:11 ` Yang Xu 0 siblings, 1 reply; 9+ messages in thread From: Cyrill Gorcunov @ 2019-07-23 7:23 UTC (permalink / raw) To: Yang Xu; +Cc: akpm, linux-kernel On Tue, Jul 23, 2019 at 11:30:53AM +0800, Yang Xu wrote: > arg2 will never < 0, for its type is 'unsigned long'. So negative > judgment is meaningless. > > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > --- > kernel/sys.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/sys.c b/kernel/sys.c > index 2969304c29fe..399457d26bef 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > error = current->timer_slack_ns; > break; > case PR_SET_TIMERSLACK: > - if (arg2 <= 0) > + if (arg2) > + current->timer_slack_ns = arg2; > + else > current->timer_slack_ns = > current->default_timer_slack_ns; > - else > - current->timer_slack_ns = arg2; > break; > case PR_MCE_KILL: > if (arg4 | arg5) From a glance it looks correct to me, but then... 1) you might simply compare with zero, iow if (arg2 == 0) instead of changing 7 lines 2) according to man page passing negative value should be acceptable, though it never worked as expected. I've been grepping "git log" for this file and the former API is coming from commit 6976675d94042fbd446231d1bd8b7de71a980ada Author: Arjan van de Ven <arjan@linux.intel.com> Date: Mon Sep 1 15:52:40 2008 -0700 hrtimer: create a "timer_slack" field in the task struct which is 11 years old by now. Nobody complained so far even when man page is saying pretty obviously PR_SET_TIMERSLACK (since Linux 2.6.28) Each thread has two associated timer slack values: a "default" value, and a "current" value. This operation sets the "current" timer slack value for the calling thread. If the nanosecond value supplied in arg2 is greater than zero, then the "current" value is set to this value. If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's "default" timer slack value. So i think to match the man page (and assuming that accepting negative value has been supposed) we should rather do if ((long)arg2 < 0) Thoughts? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK 2019-07-23 7:23 ` Cyrill Gorcunov @ 2019-07-23 8:11 ` Yang Xu 2019-07-23 9:48 ` Cyrill Gorcunov 0 siblings, 1 reply; 9+ messages in thread From: Yang Xu @ 2019-07-23 8:11 UTC (permalink / raw) To: Cyrill Gorcunov; +Cc: akpm, linux-kernel on 2019/07/23 15:23, Cyrill Gorcunov wrote: > On Tue, Jul 23, 2019 at 11:30:53AM +0800, Yang Xu wrote: >> arg2 will never< 0, for its type is 'unsigned long'. So negative >> judgment is meaningless. >> >> Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com> >> --- >> kernel/sys.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/sys.c b/kernel/sys.c >> index 2969304c29fe..399457d26bef 100644 >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -2372,11 +2372,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, >> error = current->timer_slack_ns; >> break; >> case PR_SET_TIMERSLACK: >> - if (arg2<= 0) >> + if (arg2) >> + current->timer_slack_ns = arg2; >> + else >> current->timer_slack_ns = >> current->default_timer_slack_ns; >> - else >> - current->timer_slack_ns = arg2; >> break; >> case PR_MCE_KILL: >> if (arg4 | arg5) > > From a glance it looks correct to me, but then... > > 1) you might simply compare with zero, iow if (arg2 == 0) > instead of changing 7 lines Hi Cyril Indeed. simply compare with zero might be better. > 2) according to man page passing negative value should be acceptable, > though it never worked as expected. I've been grepping "git log" > for this file and the former API is coming from > > commit 6976675d94042fbd446231d1bd8b7de71a980ada > Author: Arjan van de Ven<arjan@linux.intel.com> > Date: Mon Sep 1 15:52:40 2008 -0700 > > hrtimer: create a "timer_slack" field in the task struct > > which is 11 years old by now. Nobody complained so far even when man > page is saying pretty obviously > > PR_SET_TIMERSLACK (since Linux 2.6.28) > Each thread has two associated timer slack values: a "default" > value, and a "current" value. This operation sets the "current" > timer slack value for the calling thread. If the nanosecond > value supplied in arg2 is greater than zero, then the "current" > value is set to this value. If arg2 is less than or equal to > zero, the "current" timer slack is reset to the thread's > "default" timer slack value. > > So i think to match the man page (and assuming that accepting negative value > has been supposed) we should rather do > > if ((long)arg2< 0) Looks correct. But if we set a ULONG_MAX(PR_GET_TIMERSLACK also limits ULONG_MAX) value(about 4s) on 32bit machine, this code will think this value is a negative value and use default value. I guess man page was written as "less than or equal to zero" because of this confusing code(arg2<=0, but arg2 is an unsinged long value). I think we can change this man page and also add bounds value description. Also, I found a patch about arg2 is an unsigned long value commit 7fe5e04292e71af34ae171b88caa2a139e0b6125 Author: Chen Gang<gang.chen@asianux.com> Date: Thu Feb 21 16:43:06 2013 -0800 sys_prctl(): arg2 is unsigned long which is never< 0 arg2 will never< 0, for its type is 'unsigned long' Also, use the provided macros. What do you think about it ? > Thoughts? > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK 2019-07-23 8:11 ` Yang Xu @ 2019-07-23 9:48 ` Cyrill Gorcunov 2019-07-24 2:11 ` [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero Yang Xu 0 siblings, 1 reply; 9+ messages in thread From: Cyrill Gorcunov @ 2019-07-23 9:48 UTC (permalink / raw) To: Yang Xu; +Cc: akpm, linux-kernel On Tue, Jul 23, 2019 at 04:11:09PM +0800, Yang Xu wrote: > > 2) according to man page passing negative value should be acceptable, > > though it never worked as expected. I've been grepping "git log" > > for this file and the former API is coming from > > > > commit 6976675d94042fbd446231d1bd8b7de71a980ada > > Author: Arjan van de Ven<arjan@linux.intel.com> > > Date: Mon Sep 1 15:52:40 2008 -0700 > > > > hrtimer: create a "timer_slack" field in the task struct > > > > which is 11 years old by now. Nobody complained so far even when man > > page is saying pretty obviously > > > > PR_SET_TIMERSLACK (since Linux 2.6.28) > > Each thread has two associated timer slack values: a "default" > > value, and a "current" value. This operation sets the "current" > > timer slack value for the calling thread. If the nanosecond > > value supplied in arg2 is greater than zero, then the "current" > > value is set to this value. If arg2 is less than or equal to > > zero, the "current" timer slack is reset to the thread's > > "default" timer slack value. > > > > So i think to match the man page (and assuming that accepting negative value > > has been supposed) we should rather do > > > > if ((long)arg2< 0) > Looks correct. But if we set a ULONG_MAX(PR_GET_TIMERSLACK also limits ULONG_MAX) > value(about 4s) on 32bit machine, this code will think this value is a negative value and use default value. > > I guess man page was written as "less than or equal to zero" because of this confusing code(arg2<=0, but arg2 > is an unsinged long value). > I think we can change this man page and also add bounds value description. OK, seems reasonable. I think we should use comparision with zero and simply update a man page. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero 2019-07-23 9:48 ` Cyrill Gorcunov @ 2019-07-24 2:11 ` Yang Xu 2019-07-24 6:56 ` Cyrill Gorcunov 2019-07-25 2:14 ` Andrew Morton 0 siblings, 2 replies; 9+ messages in thread From: Yang Xu @ 2019-07-24 2:11 UTC (permalink / raw) To: akpm, gorcunov; +Cc: linux-kernel, Yang Xu Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an unsigned long value, arg2 will never < 0. Negative judgment is meaningless, so remove it. Fixes: 6976675d9404 ("hrtimer: create a "timer_slack" field in the task struct") Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> Cc: Cyrill Gorcunov <gorcunov@gmail.com> --- kernel/sys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..701b5f00651d 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = current->timer_slack_ns; break; case PR_SET_TIMERSLACK: - if (arg2 <= 0) + if (arg2 == 0) current->timer_slack_ns = current->default_timer_slack_ns; else -- 2.18.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero 2019-07-24 2:11 ` [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero Yang Xu @ 2019-07-24 6:56 ` Cyrill Gorcunov 2019-07-25 2:14 ` Andrew Morton 1 sibling, 0 replies; 9+ messages in thread From: Cyrill Gorcunov @ 2019-07-24 6:56 UTC (permalink / raw) To: Yang Xu; +Cc: akpm, linux-kernel On Wed, Jul 24, 2019 at 10:11:48AM +0800, Yang Xu wrote: > Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an > unsigned long value, arg2 will never < 0. Negative judgment is > meaningless, so remove it. > > Fixes: 6976675d9404 ("hrtimer: create a "timer_slack" field in the task struct") > Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> > Cc: Cyrill Gorcunov <gorcunov@gmail.com> Acked-by: Cyrill Gorcunov <gorcunov@gmail.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero 2019-07-24 2:11 ` [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero Yang Xu 2019-07-24 6:56 ` Cyrill Gorcunov @ 2019-07-25 2:14 ` Andrew Morton 2019-07-25 3:10 ` Yang Xu 1 sibling, 1 reply; 9+ messages in thread From: Andrew Morton @ 2019-07-25 2:14 UTC (permalink / raw) To: Yang Xu; +Cc: gorcunov, linux-kernel On Wed, 24 Jul 2019 10:11:48 +0800 Yang Xu <xuyang2018.jy@cn.fujitsu.com> wrote: > Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an > unsigned long value, arg2 will never < 0. Negative judgment is > meaningless, so remove it. > > ... > > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > error = current->timer_slack_ns; > break; > case PR_SET_TIMERSLACK: > - if (arg2 <= 0) > + if (arg2 == 0) > current->timer_slack_ns = > current->default_timer_slack_ns; A number of years ago Linus expressed approval of such comparisons with unsigned quantities. He felt that it improves readability a little - the reader doesn't have to scroll back and check the type. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero 2019-07-25 2:14 ` Andrew Morton @ 2019-07-25 3:10 ` Yang Xu 2019-07-30 8:48 ` Yang Xu 0 siblings, 1 reply; 9+ messages in thread From: Yang Xu @ 2019-07-25 3:10 UTC (permalink / raw) To: Andrew Morton; +Cc: gorcunov, linux-kernel on 2019/07/25 10:14, Andrew Morton wrote: > On Wed, 24 Jul 2019 10:11:48 +0800 Yang Xu<xuyang2018.jy@cn.fujitsu.com> wrote: > >> Currently, when calling prctl(PR_SET_TIMERSLACK, arg2), arg2 is an >> unsigned long value, arg2 will never< 0. Negative judgment is >> meaningless, so remove it. >> >> ... >> >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, >> error = current->timer_slack_ns; >> break; >> case PR_SET_TIMERSLACK: >> - if (arg2<= 0) >> + if (arg2 == 0) >> current->timer_slack_ns = >> current->default_timer_slack_ns; > A number of years ago Linus expressed approval of such comparisons with > unsigned quantities. He felt that it improves readability a little - > the reader doesn't have to scroll back and check the type. Hi Andrew It sounds good. ButWe still have to look at the actual situation. In here, this comparisons with unsigned quantities doesn't improvereadability. In turn, the code give user a wrongdescription as man page said " If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's default" timer slack value." If we set -1 in user space, we pass it into kernel as ULONG_MAX, it will not use default timer_slack value. Also, I guess that if value has no actual sense we can use this comparisons. In here, arg2 represents slack time. time will never less than 0. ps: whether we change or not change this comparisons, it doesn't affect logic. So if you think this patch is meaningless, I will accept it. Thanks Yang Xu > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero 2019-07-25 3:10 ` Yang Xu @ 2019-07-30 8:48 ` Yang Xu 0 siblings, 0 replies; 9+ messages in thread From: Yang Xu @ 2019-07-30 8:48 UTC (permalink / raw) To: Andrew Morton; +Cc: gorcunov, linux-kernel on 2019/07/25 11:10, Yang Xu wrote: >>> --- a/kernel/sys.c >>> +++ b/kernel/sys.c >>> @@ -2372,7 +2372,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, >>> error = current->timer_slack_ns; >>> break; >>> case PR_SET_TIMERSLACK: >>> - if (arg2<= 0) >>> + if (arg2 == 0) >>> current->timer_slack_ns = >>> current->default_timer_slack_ns; >> A number of years ago Linus expressed approval of such comparisons with >> unsigned quantities. He felt that it improves readability a little - >> the reader doesn't have to scroll back and check the type. > Hi Andrew > > It sounds good. ButWe still have to look at the actual situation. In here, this comparisons with unsigned > quantities doesn't improvereadability. In turn, the code give user a wrongdescription as man page said " > If arg2 is less than or equal to zero, the "current" timer slack is reset to the thread's default" timer slack value." > If we set -1 in user space, we pass it into kernel as ULONG_MAX, it will not use default timer_slack value. > Also, I guess that if value has no actual sense we can use this comparisons. In here, arg2 represents slack time. > time will never less than 0. > ps: whether we change or not change this comparisons, it doesn't affect logic. So if you think this patch is meaningless, > I will accept it. Hi Andrew what do you think about it? update it or keep it. Thanks Yang Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-30 8:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-23 3:30 [PATCH] sys_prctl(): simplify arg2 judgment when calling PR_SET_TIMERSLACK Yang Xu 2019-07-23 7:23 ` Cyrill Gorcunov 2019-07-23 8:11 ` Yang Xu 2019-07-23 9:48 ` Cyrill Gorcunov 2019-07-24 2:11 ` [PATCH v2] sys_prctl(): remove unsigned comparision with less than zero Yang Xu 2019-07-24 6:56 ` Cyrill Gorcunov 2019-07-25 2:14 ` Andrew Morton 2019-07-25 3:10 ` Yang Xu 2019-07-30 8:48 ` Yang Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).