On Wed, 27 Oct 2010, Darren Hart wrote:
> The futex_q struct has grown considerably over the last couple years. I
> believe it now merits a static initializer to avoid uninitialized data
> errors (having spent more time than I care to admit debugging an uninitialized
> q.bitset in an experimental new op code).
>
> With the key initializer built in, several of the FUTEX_KEY_INIT calls can
> be removed.
>
> Signed-off-by: Darren Hart <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: John Kacur <[email protected]>
> ---
> kernel/futex.c | 25 ++++++++++---------------
> 1 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index 8502498..4a10d44 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -131,6 +131,12 @@ struct futex_q {
> u32 bitset;
> };
>
> +#define FUTEX_Q_INIT \
> + { /* list gets initialized in queue_me()*/ \
> + .task = NULL, NULL, FUTEX_KEY_INIT \
> + , NULL, NULL, NULL, FUTEX_BITSET_MATCH_ANY }
> +
That should be a static readonly variable with a proper C99
initializer.
Thanks,
tglx
On Mon, 2010-11-08 at 17:42 +0100, Thomas Gleixner wrote:
> > +#define FUTEX_Q_INIT \
> > + { /* list gets initialized in queue_me()*/ \
> > + .task = NULL, NULL, FUTEX_KEY_INIT \
> > + , NULL, NULL, NULL, FUTEX_BITSET_MATCH_ANY }
> > +
>
> That should be a static readonly variable with a proper C99
> initializer.
Well, it doesn't need to actually be an existing variable, but yeah it
should definately use C99 initializers.
On Mon, 8 Nov 2010, Peter Zijlstra wrote:
> On Mon, 2010-11-08 at 17:42 +0100, Thomas Gleixner wrote:
> > > +#define FUTEX_Q_INIT \
> > > + { /* list gets initialized in queue_me()*/ \
> > > + .task = NULL, NULL, FUTEX_KEY_INIT \
> > > + , NULL, NULL, NULL, FUTEX_BITSET_MATCH_ANY }
> > > +
> >
> > That should be a static readonly variable with a proper C99
> > initializer.
>
> Well, it doesn't need to actually be an existing variable, but yeah it
> should definately use C99 initializers.
If you have multiple instances of q = q_init; the static variable is
more efficient vs. code/data size
Thanks,
tglx
The futex_q struct has grown considerably over the last couple years. I
believe it now merits a static initializer to avoid uninitialized data
errors (having spent more time than I care to admit debugging an uninitialized
q.bitset in an experimental new op code).
With the key initializer built in, several of the FUTEX_KEY_INIT calls can
be removed.
V2: use a static variable instead of an init macro.
use a C99 initializer and don't rely on variable ordering in the struct.
Signed-off-by: Darren Hart <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: John Kacur <[email protected]>
---
kernel/futex.c | 25 ++++++++++---------------
1 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index fd3fbe1..1eea066 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -131,6 +131,12 @@ struct futex_q {
u32 bitset;
};
+static struct futex_q futex_q_init = {
+ /* list gets initialized in queue_me()*/
+ .key = FUTEX_KEY_INIT,
+ .bitset = FUTEX_BITSET_MATCH_ANY
+};
+
/*
* Hash buckets are shared by all the futex_keys that hash to the same
* location. Each key may have multiple futex_q structures, one for each task
@@ -1751,7 +1757,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
* rare, but normal.
*/
retry:
- q->key = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q->key);
if (unlikely(ret != 0))
return ret;
@@ -1792,16 +1797,12 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
struct hrtimer_sleeper timeout, *to = NULL;
struct restart_block *restart;
struct futex_hash_bucket *hb;
- struct futex_q q;
+ struct futex_q q = futex_q_init;
int ret;
if (!bitset)
return -EINVAL;
-
- q.pi_state = NULL;
q.bitset = bitset;
- q.rt_waiter = NULL;
- q.requeue_pi_key = NULL;
if (abs_time) {
to = &timeout;
@@ -1892,7 +1893,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
{
struct hrtimer_sleeper timeout, *to = NULL;
struct futex_hash_bucket *hb;
- struct futex_q q;
+ struct futex_q q = futex_q_init;
int res, ret;
if (refill_pi_state_cache())
@@ -1906,11 +1907,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
hrtimer_set_expires(&to->timer, *time);
}
- q.pi_state = NULL;
- q.rt_waiter = NULL;
- q.requeue_pi_key = NULL;
retry:
- q.key = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key);
if (unlikely(ret != 0))
goto out;
@@ -2198,8 +2195,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
struct rt_mutex_waiter rt_waiter;
struct rt_mutex *pi_mutex = NULL;
struct futex_hash_bucket *hb;
- union futex_key key2;
- struct futex_q q;
+ union futex_key key2 = FUTEX_KEY_INIT;
+ struct futex_q q = futex_q_init;
int res, ret;
if (!bitset)
@@ -2222,12 +2219,10 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
debug_rt_mutex_init_waiter(&rt_waiter);
rt_waiter.task = NULL;
- key2 = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2);
if (unlikely(ret != 0))
goto out;
- q.pi_state = NULL;
q.bitset = bitset;
q.rt_waiter = &rt_waiter;
q.requeue_pi_key = &key2;
--
1.7.1
The futex_q struct has grown considerably over the last couple years. I
believe it now merits a static initializer to avoid uninitialized data
errors (having spent more time than I care to admit debugging an uninitialized
q.bitset in an experimental new op code).
With the key initializer built in, several of the FUTEX_KEY_INIT calls can
be removed.
V2: use a static variable instead of an init macro.
use a C99 initializer and don't rely on variable ordering in the struct.
V3: make futex_q_init const
Signed-off-by: Darren Hart <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
CC: Eric Dumazet <[email protected]>
CC: John Kacur <[email protected]>
---
kernel/futex.c | 25 ++++++++++---------------
1 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/kernel/futex.c b/kernel/futex.c
index fd3fbe1..e6df2de 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -131,6 +131,12 @@ struct futex_q {
u32 bitset;
};
+static const struct futex_q futex_q_init = {
+ /* list gets initialized in queue_me()*/
+ .key = FUTEX_KEY_INIT,
+ .bitset = FUTEX_BITSET_MATCH_ANY
+};
+
/*
* Hash buckets are shared by all the futex_keys that hash to the same
* location. Each key may have multiple futex_q structures, one for each task
@@ -1751,7 +1757,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
* rare, but normal.
*/
retry:
- q->key = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q->key);
if (unlikely(ret != 0))
return ret;
@@ -1792,16 +1797,12 @@ static int futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
struct hrtimer_sleeper timeout, *to = NULL;
struct restart_block *restart;
struct futex_hash_bucket *hb;
- struct futex_q q;
+ struct futex_q q = futex_q_init;
int ret;
if (!bitset)
return -EINVAL;
-
- q.pi_state = NULL;
q.bitset = bitset;
- q.rt_waiter = NULL;
- q.requeue_pi_key = NULL;
if (abs_time) {
to = &timeout;
@@ -1892,7 +1893,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
{
struct hrtimer_sleeper timeout, *to = NULL;
struct futex_hash_bucket *hb;
- struct futex_q q;
+ struct futex_q q = futex_q_init;
int res, ret;
if (refill_pi_state_cache())
@@ -1906,11 +1907,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
hrtimer_set_expires(&to->timer, *time);
}
- q.pi_state = NULL;
- q.rt_waiter = NULL;
- q.requeue_pi_key = NULL;
retry:
- q.key = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr, flags & FLAGS_SHARED, &q.key);
if (unlikely(ret != 0))
goto out;
@@ -2198,8 +2195,8 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
struct rt_mutex_waiter rt_waiter;
struct rt_mutex *pi_mutex = NULL;
struct futex_hash_bucket *hb;
- union futex_key key2;
- struct futex_q q;
+ union futex_key key2 = FUTEX_KEY_INIT;
+ struct futex_q q = futex_q_init;
int res, ret;
if (!bitset)
@@ -2222,12 +2219,10 @@ static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
debug_rt_mutex_init_waiter(&rt_waiter);
rt_waiter.task = NULL;
- key2 = FUTEX_KEY_INIT;
ret = get_futex_key(uaddr2, flags & FLAGS_SHARED, &key2);
if (unlikely(ret != 0))
goto out;
- q.pi_state = NULL;
q.bitset = bitset;
q.rt_waiter = &rt_waiter;
q.requeue_pi_key = &key2;
--
1.7.1
On Mon, 8 Nov 2010, Darren Hart wrote:
> The futex_q struct has grown considerably over the last couple years. I
> believe it now merits a static initializer to avoid uninitialized data
> errors (having spent more time than I care to admit debugging an uninitialized
> q.bitset in an experimental new op code).
>
> With the key initializer built in, several of the FUTEX_KEY_INIT calls can
> be removed.
>
> V2: use a static variable instead of an init macro.
> use a C99 initializer and don't rely on variable ordering in the struct.
> V3: make futex_q_init const
>
> Signed-off-by: Darren Hart <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: John Kacur <[email protected]>
> ---
> kernel/futex.c | 25 ++++++++++---------------
> 1 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/kernel/futex.c b/kernel/futex.c
> index fd3fbe1..e6df2de 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -131,6 +131,12 @@ struct futex_q {
> u32 bitset;
> };
>
> +static const struct futex_q futex_q_init = {
> + /* list gets initialized in queue_me()*/
> + .key = FUTEX_KEY_INIT,
> + .bitset = FUTEX_BITSET_MATCH_ANY
> +};
> +
> /*
> * Hash buckets are shared by all the futex_keys that hash to the same
> * location. Each key may have multiple futex_q structures, one for each task
> @@ -1751,7 +1757,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
> * rare, but normal.
> */
> retry:
> - q->key = FUTEX_KEY_INIT;
You sure about that one in the retry path ?
> @@ -1906,11 +1907,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
> hrtimer_set_expires(&to->timer, *time);
> }
>
> - q.pi_state = NULL;
> - q.rt_waiter = NULL;
> - q.requeue_pi_key = NULL;
> retry:
> - q.key = FUTEX_KEY_INIT;
Ditto
Thanks,
tglx
On 11/08/2010 01:48 PM, Thomas Gleixner wrote:
> On Mon, 8 Nov 2010, Darren Hart wrote:
>> /*
>> * Hash buckets are shared by all the futex_keys that hash to the same
>> * location. Each key may have multiple futex_q structures, one for each task
>> @@ -1751,7 +1757,6 @@ static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
>> * rare, but normal.
>> */
>> retry:
>> - q->key = FUTEX_KEY_INIT;
>
> You sure about that one in the retry path ?
>
>> @@ -1906,11 +1907,7 @@ static int futex_lock_pi(u32 __user *uaddr, unsigned int flags, int detect,
>> hrtimer_set_expires(&to->timer, *time);
>> }
>>
>> - q.pi_state = NULL;
>> - q.rt_waiter = NULL;
>> - q.requeue_pi_key = NULL;
>> retry:
>> - q.key = FUTEX_KEY_INIT;
>
> Ditto
Yes, these are fine. get_futex_key (called immediately after retry: in
both cases) will set the mm or inode or error out. On error we return
immediately. No need to zero both.ptr.
Thanks,
--
Darren Hart
Embedded Linux Kernel