Remove completion from struct klist_node
PATCH 1/1 Remove completion from struct klist_node
Removing the completion from klist_node reduces its size from 64 bytes to 28 on x86-64. To maintain the semantics of klist_remove(), we add a single list of klist nodes which are pending deletion and scan them.
include/linux/klist.h | 2 -- lib/klist.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 6 deletions(-)
include/linux/klist.h +0 -2
@@ -13,7 +13,6 @@#define _LINUX_KLIST_H#include <linux/spinlock.h> -#include <linux/completion.h> #include <linux/kref.h> #include <linux/list.h>
@@ -41,7 +40,6 @@ struct klist_node {struct klist *n_klist;struct list_head n_node;struct kref n_ref; -struct completion n_removed; };extern void klist_add_tail (struct klist_node *n,struct klist *k);
lib/klist.c +44 -4
78 lines not on this page · raw message
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 53 lines · Matthew Wilcox
Removing the completion from
klist_nodereduces its size from 64 bytes to 28 on x86-64. To maintain the semantics ofklist_remove(), we add a single list of klist nodes which are pending deletion and scan them.Signed-off-by Matthew Wilcox <[email protected]>include/linux/klist.h | 2 -- lib/klist.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 6 deletions(-)include/linux/klist.h +0
-2@@ -13,7 +13,6 @@#define _LINUX_KLIST_H#include <linux/spinlock.h> -#include <linux/completion.h> #include <linux/kref.h> #include <linux/list.h> @@ -41,7 +40,6 @@ struct klist_node {struct klist *n_klist;struct list_head n_node;struct kref n_ref; -struct completion n_removed; };extern void klist_add_tail (struct klist_node *n,struct klist *k);lib/klist.c +1
-2@@ -36,7 +36,7 @@#include <linux/klist.h> #include <linux/module.h> - +#include <linux/sched.h> /** * klist_init - Initialize a klist structure. @@ -77,7 +77,6 @@ static void add_tail(struct klist *k, struct klist_node *n)static void klist_node_init (struct klist *k,struct klist_node *n) {INIT_LIST_HEAD (&n->n_node); -init_completion (&n->n_removed);kref_init (&n->n_ref); n->n_klist = k;if (k->get)
lib/klist.c void klist_add_before(struct klist_node *n, struct klist_node *pos) · line 147
Quoted 9 lines
lib/klist.c +6
-0@@ -140,12 +139,36 @@ void klist_add_before(struct klist_node *n, struct klist_node *pos) }EXPORT_SYMBOL_GPL (klist_add_before); +struct klist_waiter { +struct klist_waiter *next; +struct klist_node *node; +struct task_struct *process; +int woken; +};
Why not use the built-in list.h functions here?
Other than that, looks good, thanks.
greg k-h
Quoted 8 lines · Greg KH
lib/klist.c void klist_add_before(struct klist_node *n, struct klist_node *pos) · line 147
Quoted 6 lines
lib/klist.c +6
-0+struct klist_waiter { +struct klist_waiter *next; +struct klist_node *node; +struct task_struct *process; +int woken; +};Why not use the built-in
list.hfunctions here?
I'm somewhat averse to using data structures when they do more than I need them to. list_heads are great for when you need to remove an entry from the middle of a list, but there are no advantages to using a doubly-linked list here -- we always walk it from the start to the end, and a singly linked list is fine for this purpose. Maybe we need a set of 'slist' macros so we can use singly-linked-lists without thinking terribly hard, but I'd hate to see this patch get stuck behind infrastructure improvements.
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 10 lines · Matthew Wilcox
Quoted 8 lines · Greg KH
lib/klist.c void klist_add_before(struct klist_node *n, struct klist_node *pos) · line 147
Quoted 6 lines
lib/klist.c +6
-0+struct klist_waiter { +struct klist_waiter *next; +struct klist_node *node; +struct task_struct *process; +int woken; +};Why not use the built-in
list.hfunctions here?
Quoted 2 lines
I'm somewhat averse to using data structures when they do more than I need them to.
But part of the point is that if you use them, no one has to audit your logic to make sure you got it all correct.
Quoted 7 lines
list_headsare great for when you need to remove an entry from the middle of a list, but there are no advantages to using a doubly-linked list here -- we always walk it from the start to the end, and a singly linked list is fine for this purpose. Maybe we need a set of 'slist' macros so we can use singly-linked-lists without thinking terribly hard, but I'd hate to see this patch get stuck behind infrastructure improvements.
Is size an issue here? I don't think so and I'd prefer it to be changed to use the standard list implementation and not roll your own. You never know who will cut-and-paste you code these days :)
Care to respin this?
thanks,
greg k-h
Quoted 1 line · Greg KH
Care to respin this?
Done:
--------
Removing the completion from klist_node reduces its size from 64 bytes to 28 on x86-64. To maintain the semantics of klist_remove(), we add a single list of klist nodes which are pending deletion and scan them.
include/linux/klist.h +0 -2
@@ -13,7 +13,6 @@#define _LINUX_KLIST_H#include <linux/spinlock.h> -#include <linux/completion.h> #include <linux/kref.h> #include <linux/list.h>
@@ -41,7 +40,6 @@ struct klist_node {struct klist *n_klist;struct list_head n_node;struct kref n_ref; -struct completion n_removed; };extern void klist_add_tail (struct klist_node *n,struct klist *k);
lib/klist.c +40 -4
74 lines not on this page · raw message
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."