Re: [RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Waiman Long]

On 09/22/2016 05:31 PM, Thomas Gleixner wrote:

On Tue, 20 Sep 2016, Waiman Long wrote:

Please be more careful of your subject lines. First thing I thought was
that you add a helper which is used in later patches to find out that you
actualy consolidate duplicated code. Something like:

   futex: Consolidate duplicated timer setup code

would have told me right away what this is about.


This patch adds a new futex_set_timer() function to consolidate all

Please do not use: "This patch ...". We already know that this is a patch,
otherwise it would not be tagged [PATCH n/m] in the subject line.

See Documentation/SubmittingPatches 


the sleeping hrtime setup code.

Let me give you a hint:

1:  The code has three identical code copies to set up the futex timeout.

2:  Add a helper function and consolidate the call sites.

#1 tells precisely what the problem is
#2 tells precisely how it is solved

Can you see the difference?


+/*
+ * Helper function to set the sleeping hrtimer.
+ */
+static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper **pto,
+   struct hrtimer_sleeper *timeout, int flags, u64 range_ns)

Please use futex_setup_timer() as the function name. I was confused when I
read the other patch that you wanted to "set" the timer before entering
into the place which would actually need it.


+{
+   if (!time)
+   return;
+   *pto = timeout;

Please don't do that. That's a horrible coding style.

What's wrong with returning NULL or the timeout pointer and assign it to
"to" at the call site?

Thanks,

tglx



Thanks for the suggestions. I will fix this patch in the next revision.

Cheers,
Longman

Re: [RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Waiman Long]

On 09/22/2016 05:31 PM, Thomas Gleixner wrote:

On Tue, 20 Sep 2016, Waiman Long wrote:

Please be more careful of your subject lines. First thing I thought was
that you add a helper which is used in later patches to find out that you
actualy consolidate duplicated code. Something like:

   futex: Consolidate duplicated timer setup code

would have told me right away what this is about.


This patch adds a new futex_set_timer() function to consolidate all

Please do not use: "This patch ...". We already know that this is a patch,
otherwise it would not be tagged [PATCH n/m] in the subject line.

See Documentation/SubmittingPatches 


the sleeping hrtime setup code.

Let me give you a hint:

1:  The code has three identical code copies to set up the futex timeout.

2:  Add a helper function and consolidate the call sites.

#1 tells precisely what the problem is
#2 tells precisely how it is solved

Can you see the difference?


+/*
+ * Helper function to set the sleeping hrtimer.
+ */
+static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper **pto,
+   struct hrtimer_sleeper *timeout, int flags, u64 range_ns)

Please use futex_setup_timer() as the function name. I was confused when I
read the other patch that you wanted to "set" the timer before entering
into the place which would actually need it.


+{
+   if (!time)
+   return;
+   *pto = timeout;

Please don't do that. That's a horrible coding style.

What's wrong with returning NULL or the timeout pointer and assign it to
"to" at the call site?

Thanks,

tglx



Thanks for the suggestions. I will fix this patch in the next revision.

Cheers,
Longman

Re: [RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Thomas Gleixner]

On Tue, 20 Sep 2016, Waiman Long wrote:

Please be more careful of your subject lines. First thing I thought was
that you add a helper which is used in later patches to find out that you
actualy consolidate duplicated code. Something like:

  futex: Consolidate duplicated timer setup code

would have told me right away what this is about.

> This patch adds a new futex_set_timer() function to consolidate all

Please do not use: "This patch ...". We already know that this is a patch,
otherwise it would not be tagged [PATCH n/m] in the subject line.

See Documentation/SubmittingPatches 

> the sleeping hrtime setup code.

Let me give you a hint:

1:  The code has three identical code copies to set up the futex timeout.

2:  Add a helper function and consolidate the call sites.

#1 tells precisely what the problem is
#2 tells precisely how it is solved

Can you see the difference?

> +/*
> + * Helper function to set the sleeping hrtimer.
> + */
> +static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper 
> **pto,
> + struct hrtimer_sleeper *timeout, int flags, u64 range_ns)

Please use futex_setup_timer() as the function name. I was confused when I
read the other patch that you wanted to "set" the timer before entering
into the place which would actually need it.

> +{
> + if (!time)
> + return;
> + *pto = timeout;

Please don't do that. That's a horrible coding style.

What's wrong with returning NULL or the timeout pointer and assign it to
"to" at the call site?

Thanks,

tglx

Re: [RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Thomas Gleixner]

On Tue, 20 Sep 2016, Waiman Long wrote:

Please be more careful of your subject lines. First thing I thought was
that you add a helper which is used in later patches to find out that you
actualy consolidate duplicated code. Something like:

  futex: Consolidate duplicated timer setup code

would have told me right away what this is about.

> This patch adds a new futex_set_timer() function to consolidate all

Please do not use: "This patch ...". We already know that this is a patch,
otherwise it would not be tagged [PATCH n/m] in the subject line.

See Documentation/SubmittingPatches 

> the sleeping hrtime setup code.

Let me give you a hint:

1:  The code has three identical code copies to set up the futex timeout.

2:  Add a helper function and consolidate the call sites.

#1 tells precisely what the problem is
#2 tells precisely how it is solved

Can you see the difference?

> +/*
> + * Helper function to set the sleeping hrtimer.
> + */
> +static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper 
> **pto,
> + struct hrtimer_sleeper *timeout, int flags, u64 range_ns)

Please use futex_setup_timer() as the function name. I was confused when I
read the other patch that you wanted to "set" the timer before entering
into the place which would actually need it.

> +{
> + if (!time)
> + return;
> + *pto = timeout;

Please don't do that. That's a horrible coding style.

What's wrong with returning NULL or the timeout pointer and assign it to
"to" at the call site?

Thanks,

tglx

[RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Waiman Long]

This patch adds a new futex_set_timer() function to consolidate all
the sleeping hrtime setup code.

Signed-off-by: Waiman Long 
---
 kernel/futex.c |   51 ---
 1 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 46cb3a3..37e61ef 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -467,6 +467,25 @@ static void drop_futex_key_refs(union futex_key *key)
}
 }
 
+/*
+ * Helper function to set the sleeping hrtimer.
+ */
+static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper **pto,
+   struct hrtimer_sleeper *timeout, int flags, u64 range_ns)
+{
+   if (!time)
+   return;
+   *pto = timeout;
+   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
+ CLOCK_REALTIME : CLOCK_MONOTONIC,
+ HRTIMER_MODE_ABS);
+   hrtimer_init_sleeper(timeout, current);
+   if (range_ns)
+   hrtimer_set_expires_range_ns(>timer, *time, range_ns);
+   else
+   hrtimer_set_expires(>timer, *time);
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr: virtual address of the futex
@@ -2403,17 +2422,8 @@ static int futex_wait(u32 __user *uaddr, unsigned int 
flags, u32 val,
return -EINVAL;
q.bitset = bitset;
 
-   if (abs_time) {
-   to = 
-
-   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
- CLOCK_REALTIME : CLOCK_MONOTONIC,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires_range_ns(>timer, *abs_time,
-current->timer_slack_ns);
-   }
-
+   futex_set_timer(abs_time, , , flags,
+   current->timer_slack_ns);
 retry:
/*
 * Prepare to wait on uaddr. On success, holds hb lock and increments
@@ -2501,13 +2511,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int 
flags,
if (refill_pi_state_cache())
return -ENOMEM;
 
-   if (time) {
-   to = 
-   hrtimer_init_on_stack(>timer, CLOCK_REALTIME,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires(>timer, *time);
-   }
+   futex_set_timer(time, , , FLAGS_CLOCKRT, 0);
 
 retry:
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, , VERIFY_WRITE);
@@ -2816,15 +2820,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, 
unsigned int flags,
if (!bitset)
return -EINVAL;
 
-   if (abs_time) {
-   to = 
-   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
- CLOCK_REALTIME : CLOCK_MONOTONIC,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires_range_ns(>timer, *abs_time,
-current->timer_slack_ns);
-   }
+   futex_set_timer(abs_time, , , flags,
+   current->timer_slack_ns);
 
/*
 * The waiter is allocated on our stack, manipulated by the requeue
-- 
1.7.1

[RFC PATCH v2 1/5] futex: Add futex_set_timer() helper function

[Waiman Long]

This patch adds a new futex_set_timer() function to consolidate all
the sleeping hrtime setup code.

Signed-off-by: Waiman Long 
---
 kernel/futex.c |   51 ---
 1 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/kernel/futex.c b/kernel/futex.c
index 46cb3a3..37e61ef 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -467,6 +467,25 @@ static void drop_futex_key_refs(union futex_key *key)
}
 }
 
+/*
+ * Helper function to set the sleeping hrtimer.
+ */
+static inline void futex_set_timer(ktime_t *time, struct hrtimer_sleeper **pto,
+   struct hrtimer_sleeper *timeout, int flags, u64 range_ns)
+{
+   if (!time)
+   return;
+   *pto = timeout;
+   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
+ CLOCK_REALTIME : CLOCK_MONOTONIC,
+ HRTIMER_MODE_ABS);
+   hrtimer_init_sleeper(timeout, current);
+   if (range_ns)
+   hrtimer_set_expires_range_ns(>timer, *time, range_ns);
+   else
+   hrtimer_set_expires(>timer, *time);
+}
+
 /**
  * get_futex_key() - Get parameters which are the keys for a futex
  * @uaddr: virtual address of the futex
@@ -2403,17 +2422,8 @@ static int futex_wait(u32 __user *uaddr, unsigned int 
flags, u32 val,
return -EINVAL;
q.bitset = bitset;
 
-   if (abs_time) {
-   to = 
-
-   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
- CLOCK_REALTIME : CLOCK_MONOTONIC,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires_range_ns(>timer, *abs_time,
-current->timer_slack_ns);
-   }
-
+   futex_set_timer(abs_time, , , flags,
+   current->timer_slack_ns);
 retry:
/*
 * Prepare to wait on uaddr. On success, holds hb lock and increments
@@ -2501,13 +2511,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int 
flags,
if (refill_pi_state_cache())
return -ENOMEM;
 
-   if (time) {
-   to = 
-   hrtimer_init_on_stack(>timer, CLOCK_REALTIME,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires(>timer, *time);
-   }
+   futex_set_timer(time, , , FLAGS_CLOCKRT, 0);
 
 retry:
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, , VERIFY_WRITE);
@@ -2816,15 +2820,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, 
unsigned int flags,
if (!bitset)
return -EINVAL;
 
-   if (abs_time) {
-   to = 
-   hrtimer_init_on_stack(>timer, (flags & FLAGS_CLOCKRT) ?
- CLOCK_REALTIME : CLOCK_MONOTONIC,
- HRTIMER_MODE_ABS);
-   hrtimer_init_sleeper(to, current);
-   hrtimer_set_expires_range_ns(>timer, *abs_time,
-current->timer_slack_ns);
-   }
+   futex_set_timer(abs_time, , , flags,
+   current->timer_slack_ns);
 
/*
 * The waiter is allocated on our stack, manipulated by the requeue
-- 
1.7.1