plist: pass the real plist_head to plist_del()
PATCH 1/3 plist: pass the real plist_head to plist_del()
These patches shrink the struct plist_head. After it is shrinked plist_del() required a real plist_head passed into.
My tests did not cover all paths.
Subject: plist: pass the real plist_head to plist_del()
Some plist_del()s in kernel/futex.c are passed a faked plist_head.
It can work because current code does not require real plist_head in plist_del(). But it is an undocumented usage, it is not good.
---
kernel/futex.c +14 -5
@@ -158,6 +158,15 @@ static inline int match_futex(union futex_key *key1, union futex_key *key2) }/* +* find the bucket of a futex entry. +* the same as hash_futex(&q->key) but a little more effcient +*/ +static struct futex_hash_bucket *futex_bucket (struct futex_q *q) +{ + return container_of(q->lock_ptr, struct futex_hash_bucket, lock); +} + +/* * Take a reference to the resource addressed by a key. * Can be called while holding spinlocks. *
@@ -744,7 +753,7 @@ static void wake_futex(struct futex_q *q)*/ get_task_struct (p); -plist_del (&q->list, &q->list.plist); +plist_del (&q->list, &futex_bucket (q)->chain);/* * The waiting task can free the futex_q as soon as * q->lock_ptr = NULL is written, without taking any locks. A
@@ -1053,7 +1062,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key, q->key = *key;WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list, &q->list.plist); +plist_del (&q->list, &futex_bucket (q)->chain);WARN_ON (!q->rt_waiter); q->rt_waiter =NULL ;
@@ -1457,7 +1466,7 @@ retry: goto retry; }WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list, &q->list.plist); +plist_del (&q->list, &futex_bucket (q)->chain);BUG_ON (q->pi_state);
@@ -1478,7 +1487,7 @@ static void unqueue_me_pi(struct futex_q *q) __releases(q->lock_ptr) {WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list, &q->list.plist); +plist_del (&q->list, &futex_bucket (q)->chain);BUG_ON (!q->pi_state);free_pi_state (q->pi_state);
@@ -2145,7 +2154,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,* We were woken prior to requeue by a timeout or a signal. * Unqueue the futex_q and determine which it was. */ -plist_del (&q->list, &q->list.plist); +plist_del (&q->list, &hb->chain);/* Handle spurious wakeups gracefully */ ret = -EWOULDBLOCK ;
Quoted 9 lines · Lai Jiangshan
These patches shrink the struct
plist_head. After it is shrinkedplist_del()required a realplist_headpassed into.My tests did not cover all paths.
Subject: plist: pass the real
plist_headtoplist_del()Some plist_del()s in
kernel/futex.care passed a fakedplist_head.
Can you explain what you mean by a "faked plist_head"? I'm looking at the existing source and q.list.plist is a "struct plist_head". Is it the required knowledge of the implementation of a plist_node that the current implementation depends on that you object to? Effectively a lack of encapsulation?
Quoted 3 lines
It can work because current code does not require real
plist_headinplist_del(). But it is an undocumented usage, it is not good.
Please compare the results of the futextest performance tests before and after your applied patch and report the results with the patch:
git://git.kernel.org/pub/scm/linux/kernel/git/dvhart/futextest.git
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 162
Quoted 13 lines
Signed-off-by Lai Jiangshan<[email protected]>---
kernel/futex.c +2
-0@@ -158,6 +158,15 @@ static inline int match_futex(union futex_key *key1, union futex_key *key2) }/* +* find the bucket of a futex entry. +* the same as hash_futex(&q->key) but a little more effcient
efficient.
Please capitalize the first word in each sentence.
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 163
Quoted 1 line
kernel/futex.c +1
-0+*/
Please use proper kernel doc formatting for any new functions added to futex.c.
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 167
Quoted 4 lines
kernel/futex.c +4
-0+static struct futex_hash_bucket *futex_bucket (struct futex_q *q) +{ + return container_of(q->lock_ptr, struct futex_hash_bucket, lock); +}
Have you found the jhash2 to be a bottleneck in a particular path? Or was it an optimization to reduce the impact of the changes below (I'm assuming the latter).
kernel/futex.c static void wake_futex(struct futex_q *q) · line 756
Quoted 11 lines
kernel/futex.c +3
-1+ +/* * Take a reference to the resource addressed by a key. * Can be called while holding spinlocks. * @@ -744,7 +753,7 @@ static void wake_futex(struct futex_q *q)*/ get_task_struct (p); -plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);
Space after the comma while you're at it.
kernel/futex.c void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key, · line 1065
Quoted 9 lines
kernel/futex.c +1
-1/* * The waiting task can free the futex_q as soon as * q->lock_ptr = NULL is written, without taking any locks. A @@ -1053,7 +1062,7 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key, q->key = *key;WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);
Space after the comma while you're at it.
kernel/futex.c retry: · line 1469
Quoted 9 lines
kernel/futex.c +1
-1WARN_ON (!q->rt_waiter); q->rt_waiter =NULL ;@@ -1457,7 +1466,7 @@ retry: goto retry; }WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);
Space after the comma while you're at it.
kernel/futex.c static void unqueue_me_pi(struct futex_q *q) · line 1490
Quoted 8 lines
kernel/futex.c +1
-1BUG_ON (q->pi_state);@@ -1478,7 +1487,7 @@ static void unqueue_me_pi(struct futex_q *q) __releases(q->lock_ptr) {WARN_ON (plist_node_empty (&q->list)); -plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);
Space after the comma while you're at it.
kernel/futex.c int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb, · line 2157
Quoted 8 lines
kernel/futex.c +1
-1BUG_ON (!q->pi_state);free_pi_state (q->pi_state);@@ -2145,7 +2154,7 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,* We were woken prior to requeue by a timeout or a signal. * Unqueue the futex_q and determine which it was. */ -plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&hb->chain);
Space after the comma while you're at it.
Quoted 2 lines
kernel/futex.c +0
-0/* Handle spurious wakeups gracefully */ ret = -EWOULDBLOCK ;
Signature · 2 lines
Darren Hart Yocto Linux Kernel
Quoted 9 lines · Darren Hart
Quoted 8 lines · Lai Jiangshan
These patches shrink the struct
plist_head. After it is shrinkedplist_del()required a realplist_headpassed into.My tests did not cover all paths.
Subject: plist: pass the real
plist_headtoplist_del()
Quoted 7 lines
Quoted 1 line
Some plist_del()s in
kernel/futex.care passed a fakedplist_head.Can you explain what you mean by a "faked
plist_head"? I'm looking at the existing source and q.list.plist is a "structplist_head". Is it the required knowledge of the implementation of aplist_nodethat the current implementation depends on that you object to? Effectively a lack of encapsulation?
What I said is "a faked head of a priority list", I am sorry that "faked plist_head" misled you.
/**
* plist_del - Remove a @node from plist.
*
* @node: &struct plist_node pointer - entry to be removed
* @head: &struct plist_head pointer - list head
*/
The document said that @head is "list head" the head of the priority list. But the code of plist_del() just uses the head for checking, so it will not cause bad result even when we use a faked head.
In "plist_del(&q->list, &q->list.plist);", &q->list.plist is indeed a "struct plist_head", but it is not the real head of the priority list.
I think this is bad code. We should pass the real head OR just remove this parameter since it is not required real one. But my next patch will use this "list head", so this patch fixes futex.c and pass the real head to plist_del().
Quoted 11 lines
Quoted 4 lines
It can work because current code does not require real
plist_headinplist_del(). But it is an undocumented usage, it is not good.Please compare the results of the futextest performance tests before and after your applied patch and report the results with the patch:
git://git.kernel.org/pub/scm/linux/kernel/git/dvhart/futextest.git
I thought it is a clean up patch. But where are the testcases? Are they included in LTP?
Quoted 18 lines
Quoted 9 lines
Signed-off-by Lai Jiangshan<[email protected]>---
kernel/futex.c +0
-0kernel/futex.c static inline int match_futex(union futex_key · line 163
Quoted 6 lines
kernel/futex.c +2
-0@@ -158,6 +158,15 @@ static inline int match_futex(union futex_key *key1, union futex_key *key2) }/* +* find the bucket of a futex entry. +* the same as hash_futex(&q->key) but a little more effcient efficient.
Quoted 7 lines
Please capitalize the first word in each sentence.
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 163
Quoted 1 line
kernel/futex.c +1
-0+*/ Please use proper kernel doc formatting for any new functions added to
futex.c.
Will fix it.
Quoted 12 lines
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 167
Quoted 6 lines
kernel/futex.c +4
-0+static struct futex_hash_bucket *futex_bucket (struct futex_q *q) +{ + return container_of(q->lock_ptr, struct futex_hash_bucket, lock); +}Have you found the jhash2 to be a bottleneck in a particular path? Or was it an optimization to reduce the impact of the changes below (I'm assuming the latter).
jhash2() need tens of instructions after compiled. container_of() is just a ADD/SUB operation instruction.
It is OK to use hash_futex(&q->key) instead.
Quoted 10 lines
Quoted 10 lines
kernel/futex.c +2
-0+ +/* * Take a reference to the resource addressed by a key. * Can be called while holding spinlocks. *kernel/futex.c +0
-0@@ -744,7 +753,7 @@ static void wake_futex(struct futex_q *q)*/ get_task_struct (p);
Quoted 6 lines
kernel/futex.c static void wake_futex(struct futex_q *q) · line 756
Quoted 2 lines
kernel/futex.c +1
-1-plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);Space after the comma while you're at it.
Space is existed in the patch mail I received from LKML. It is strange.
Thanks,
Lai.
Quoted 29 lines · Lai Jiangshan
Quoted 9 lines · Darren Hart
Quoted 8 lines · Lai Jiangshan
These patches shrink the struct
plist_head. After it is shrinkedplist_del()required a realplist_headpassed into.My tests did not cover all paths.
Subject: plist: pass the real
plist_headtoplist_del()Quoted 7 lines
Quoted 1 line
Some plist_del()s in
kernel/futex.care passed a fakedplist_head.Can you explain what you mean by a "faked
plist_head"? I'm looking at the existing source and q.list.plist is a "structplist_head". Is it the required knowledge of the implementation of aplist_nodethat the current implementation depends on that you object to? Effectively a lack of encapsulation?What I said is "a faked head of a priority list", I am sorry that "faked
plist_head" misled you./**
*plist_del- Remove a @node from plist.
*
* @node: &structplist_nodepointer - entry to be removed
* @head: &structplist_headpointer - list head
*/
Quoted 3 lines
The document said that @head is "list head" the head of the priority list. But the code of
plist_del()just uses the head for checking, so it will not cause bad result even when we use a faked head.
Looks OK to me - I recall having issues with the PLIST_DEBUG stuff, this might have been why.
Quoted 16 lines
In "plist_del(&q->list,&q->list.plist);",&q->list.plist is indeed a "struct
plist_head", but it is not the real head of the priority list.I think this is bad code. We should pass the real head OR just remove this parameter since it is not required real one. But my next patch will use this "list head", so this patch fixes
futex.cand pass the real head toplist_del().Quoted 6 lines
Quoted 4 lines
It can work because current code does not require real
plist_headinplist_del(). But it is an undocumented usage, it is not good.
Quoted 8 lines
Quoted 5 lines
Please compare the results of the futextest performance tests before and after your applied patch and report the results with the patch:
git://git.kernel.org/pub/scm/linux/kernel/git/dvhart/futextest.gitI thought it is a clean up patch. But where are the testcases? Are they included in LTP?
These testcases are hosted at the git URL above, they are not included in LTP. Just clone the repo and build and run the tests.
Quoted 18 lines
Quoted 18 lines
kernel/futex.c static inline int match_futex(union futex_key · line 163
Quoted 13 lines
Signed-off-by Lai Jiangshan<[email protected]>---
kernel/futex.c +2
-0@@ -158,6 +158,15 @@ static inline int match_futex(union futex_key *key1, union futex_key *key2) }/* +* find the bucket of a futex entry. +* the same as hash_futex(&q->key) but a little more effcient efficient.
Please capitalize the first word in each sentence.
Quoted 8 lines
Quoted 5 lines
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 163
Quoted 1 line
kernel/futex.c +1
-0+*/ Please use proper kernel doc formatting for any new functions added to
futex.c.Will fix it.
Be sure to state any assumptions you're making about the arguments. For example, q->lock_ptr != NULL.
Quoted 13 lines
Quoted 12 lines
kernel/futex.c static inline int match_futex(union futex_key *key1, union futex_key *key2) · line 167
Quoted 6 lines
kernel/futex.c +4
-0+static struct futex_hash_bucket *futex_bucket (struct futex_q *q) +{ + return container_of(q->lock_ptr, struct futex_hash_bucket, lock); +}Have you found the jhash2 to be a bottleneck in a particular path? Or was it an optimization to reduce the impact of the changes below (I'm assuming the latter).
Quoted 4 lines
jhash2()need tens of instructions after compiled.container_of()is just a ADD/SUB operation instruction.It is OK to use
hash_futex(&q->key)instead.
I think your new routine makes sense in light of the changes to plist_del usage below. Rather than "bucket" which we don't use in the source, let's go with the following which is closer in keeping with the futex function and type naming conventions:
lookup_futex_hb()
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
kernel/futex.c +2
-0+ +/* * Take a reference to the resource addressed by a key. * Can be called while holding spinlocks. *kernel/futex.c +0
-0@@ -744,7 +753,7 @@ static void wake_futex(struct futex_q *q)*/ get_task_struct (p);
Quoted 2 lines
Quoted 2 lines
kernel/futex.c static void wake_futex(struct futex_q *q) · line 756
Quoted 2 lines
kernel/futex.c +1
-1-plist_del (&q->list,&q->list.plist); +plist_del (&q->list,&futex_bucket (q)->chain);
Hrm, in some places around uses of plist_del, we do a WARN_ON(q0>lock_ptr && ...) which means there is some concern that lock_ptr is NULL, if it is, your call here will dereference NULL and panic.
Quoted 6 lines
Quoted 4 lines
Space after the comma while you're at it.
Space is existed in the patch mail I received from LKML. It is strange.
Ugh... perhaps something on my end...
Thanks,
Signature · 2 lines
Darren Hart Yocto Linux Kernel