[PATCH] shrink task_struct by 16 bytes

[PATCH] shrink task_struct by 16 bytes

[Davi Arnaut]

Hi,

Shrink task_struct by replacing the notifier callback with a
notifier list. The block_all_signals() function (and the signal
notifier mechanism) has only one user at the moment, which is drm.

Pahole output for task_struct:

i386 before:

   /* size: 2640, cachelines: 42 */
   /* sum members: 2619, holes: 3, sum holes: 9 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 12 */
   /* paddings: 1, sum paddings: 8 */
   /* last cacheline: 16 bytes */

i386 after:

   /* size: 2624, cachelines: 41 */
   /* sum members: 2615, holes: 3, sum holes: 9 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* paddings: 1, sum paddings: 8 */

x86_64 before:

   /* size: 3648, cachelines: 57 */
   /* sum members: 3587, holes: 13, sum holes: 53 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 8 */

x86_64 after:

   /* size: 3632, cachelines: 57 */
   /* sum members: 3579, holes: 13, sum holes: 53 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* last cacheline: 48 bytes */

Signed-off-by: Davi E. M. Arnaut <davi@haxent.com.br>

---
 drivers/char/drm/drmP.h     |    4 ++--
 drivers/char/drm/drm_lock.c |   16 +++++++++-------
 include/linux/init_task.h   |    1 +
 include/linux/sched.h       |   12 +++++-------
 include/linux/signal.h      |   16 ++++++++++++++++
 kernel/fork.c               |    1 +
 kernel/signal.c             |   42 ++++++++++++++++++++----------------------
 7 files changed, 54 insertions(+), 38 deletions(-)

Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -963,10 +963,9 @@ struct task_struct {
 
 	unsigned long sas_ss_sp;
 	size_t sas_ss_size;
-	int (*notifier)(void *priv);
-	void *notifier_data;
-	sigset_t *notifier_mask;
-	
+	/* signal notifier list */
+	struct list_head notifier_list;
+
 	void *security;
 	struct audit_context *audit_context;
 	seccomp_t seccomp;
@@ -1339,9 +1338,8 @@ static inline int dequeue_signal_lock(st
 	return ret;
 }	
 
-extern void block_all_signals(int (*notifier)(void *priv), void *priv,
-			      sigset_t *mask);
-extern void unblock_all_signals(void);
+extern void block_signals(struct signal_notifier *);
+extern void unblock_signals(struct signal_notifier *);
 extern void release_task(struct task_struct * p);
 extern int send_sig_info(int, struct siginfo *, struct task_struct *);
 extern int send_group_sig_info(int, struct siginfo *, struct task_struct *);
Index: linux-2.6/include/linux/init_task.h
===================================================================
--- linux-2.6.orig/include/linux/init_task.h
+++ linux-2.6/include/linux/init_task.h
@@ -157,6 +157,7 @@ extern struct group_info init_groups;
 		.list = LIST_HEAD_INIT(tsk.pending.list),		\
 		.signal = {{0}}},					\
 	.blocked	= {{0}},					\
+	.notifier_list	= LIST_HEAD_INIT(tsk.notifier_list),		\
 	.alloc_lock	= __SPIN_LOCK_UNLOCKED(tsk.alloc_lock),		\
 	.journal_info	= NULL,						\
 	.cpu_timers	= INIT_CPU_TIMERS(tsk.cpu_timers),		\
Index: linux-2.6/include/linux/signal.h
===================================================================
--- linux-2.6.orig/include/linux/signal.h
+++ linux-2.6/include/linux/signal.h
@@ -27,6 +27,22 @@ struct sigpending {
 	sigset_t signal;
 };
 
+/**
+ * struct signal_notifier - the signal notifier struct
+ * @list: list head to enqueue into the process notifier list
+ * @func: notifier callback function. if the callback routine returns 0
+ * then then signal will be blocked.
+ * @data: pointer to private data that the notifier routine can use to
+ * determine if the signal should be blocked or not.
+ * @mask: mask of signals to call the callback routine upon
+ */
+struct signal_notifier {
+	struct list_head list;
+	int (*func)(void *priv);
+	void *data;
+	sigset_t mask;
+};
+
 /*
  * Define some primitives to manipulate sigset_t.
  */
Index: linux-2.6/kernel/fork.c
===================================================================
--- linux-2.6.orig/kernel/fork.c
+++ linux-2.6/kernel/fork.c
@@ -1034,6 +1034,7 @@ static struct task_struct *copy_process(
 	p->vfork_done = NULL;
 	spin_lock_init(&p->alloc_lock);
 
+	INIT_LIST_HEAD(&p->notifier_list);
 	clear_tsk_thread_flag(p, TIF_SIGPENDING);
 	init_sigpending(&p->pending);
 
Index: linux-2.6/drivers/char/drm/drm_lock.c
===================================================================
--- linux-2.6.orig/drivers/char/drm/drm_lock.c
+++ linux-2.6/drivers/char/drm/drm_lock.c
@@ -110,14 +110,16 @@ int drm_lock(struct inode *inode, struct
 	DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
 	if (ret) return ret;
 
-	sigemptyset(&dev->sigmask);
-	sigaddset(&dev->sigmask, SIGSTOP);
-	sigaddset(&dev->sigmask, SIGTSTP);
-	sigaddset(&dev->sigmask, SIGTTIN);
-	sigaddset(&dev->sigmask, SIGTTOU);
+	sigemptyset(&dev->notifier.mask);
+	sigaddset(&dev->notifier.mask, SIGSTOP);
+	sigaddset(&dev->notifier.mask, SIGTSTP);
+	sigaddset(&dev->notifier.mask, SIGTTIN);
+	sigaddset(&dev->notifier.mask, SIGTTOU);
 	dev->sigdata.context = lock.context;
 	dev->sigdata.lock = dev->lock.hw_lock;
-	block_all_signals(drm_notifier, &dev->sigdata, &dev->sigmask);
+	dev->notifier.data = &dev->sigdata;
+	dev->notifier.func = drm_notifier;
+	block_signals(&dev->notifier);
 
 	if (dev->driver->dma_ready && (lock.flags & _DRM_LOCK_READY))
 		dev->driver->dma_ready(dev);
@@ -189,7 +191,7 @@ int drm_unlock(struct inode *inode, stru
 		}
 	}
 
-	unblock_all_signals();
+	unblock_signals(&dev->notifier);
 	return 0;
 }
 
Index: linux-2.6/drivers/char/drm/drmP.h
===================================================================
--- linux-2.6.orig/drivers/char/drm/drmP.h
+++ linux-2.6/drivers/char/drm/drmP.h
@@ -750,8 +750,8 @@ typedef struct drm_device {
 	drm_sg_mem_t *sg;	/**< Scatter gather memory */
 	unsigned long *ctx_bitmap;	/**< context bitmap */
 	void *dev_private;		/**< device private data */
-	drm_sigdata_t sigdata;	   /**< For block_all_signals */
-	sigset_t sigmask;
+	drm_sigdata_t sigdata;	   /**< Signal notifier data */
+	struct signal_notifier notifier;	/**< For block_signals */
 
 	struct drm_driver *driver;
 	drm_local_map_t *agp_buffer_map;
Index: linux-2.6/kernel/signal.c
===================================================================
--- linux-2.6.orig/kernel/signal.c
+++ linux-2.6/kernel/signal.c
@@ -242,32 +242,28 @@ flush_signal_handlers(struct task_struct
  * process, and wants to be notified if any signals at all were to be
  * sent/acted upon.  If the notifier routine returns non-zero, then the
  * signal will be acted upon after all.  If the notifier routine returns 0,
- * then then signal will be blocked.  Only one block per process is
- * allowed.  priv is a pointer to private data that the notifier routine
- * can use to determine if the signal should be blocked or not.  */
+ * the signal will be blocked.
+ */
 
 void
-block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
+block_signals(struct signal_notifier *notifier)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier_mask = mask;
-	current->notifier_data = priv;
-	current->notifier = notifier;
+	list_add_tail(&notifier->list, &current->notifier_list);
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 }
 
 /* Notify the system that blocking has ended. */
 
 void
-unblock_all_signals(void)
+unblock_signals(struct signal_notifier *notifier)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier = NULL;
-	current->notifier_data = NULL;
+	list_del(&notifier->list);
 	recalc_sigpending();
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 }
@@ -318,22 +314,24 @@ static int collect_signal(int sig, struc
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
 			siginfo_t *info)
 {
+	struct signal_notifier *tmp, *notifier;
 	int sig = next_signal(pending, mask);
 
-	if (sig) {
-		if (current->notifier) {
-			if (sigismember(current->notifier_mask, sig)) {
-				if (!(current->notifier)(current->notifier_data)) {
-					clear_thread_flag(TIF_SIGPENDING);
-					return 0;
-				}
+	if (unlikely(!sig))
+		return 0;
+
+	list_for_each_entry_safe(notifier, tmp, &current->notifier_list, list) {
+		if (sigismember(&notifier->mask, sig)) {
+			if (!(notifier->func)(notifier->data)) {
+				clear_thread_flag(TIF_SIGPENDING);
+				return 0;
 			}
 		}
-
-		if (!collect_signal(sig, pending, info))
-			sig = 0;
 	}
 
+	if (!collect_signal(sig, pending, info))
+		sig = 0;
+
 	return sig;
 }
 
@@ -1860,8 +1858,8 @@ EXPORT_SYMBOL(ptrace_notify);
 EXPORT_SYMBOL(send_sig);
 EXPORT_SYMBOL(send_sig_info);
 EXPORT_SYMBOL(sigprocmask);
-EXPORT_SYMBOL(block_all_signals);
-EXPORT_SYMBOL(unblock_all_signals);
+EXPORT_SYMBOL(block_signals);
+EXPORT_SYMBOL(unblock_signals);
 
 
 /*

Re: [PATCH] shrink task_struct by 16 bytes

[Christoph Hellwig]

On Sun, May 20, 2007 at 12:40:11AM -0300, Davi Arnaut wrote:
> Hi,
> 
> Shrink task_struct by replacing the notifier callback with a
> notifier list. The block_all_signals() function (and the signal
> notifier mechanism) has only one user at the moment, which is drm.

This looks like a nice improvement.  Some comments below:

> -	int (*notifier)(void *priv);
> -	void *notifier_data;
> -	sigset_t *notifier_mask;
> -	
> +	/* signal notifier list */
> +	struct list_head notifier_list;
> +

This filed should have a more descripvtive name, e.g. singal_notifier_list.
Also it's probably fine to use a hlist to save another pointer in
struct task_struct.

> +	struct signal_notifier *tmp, *notifier;
>  	int sig = next_signal(pending, mask);
>  
> -	if (sig) {
> -		if (current->notifier) {
> -			if (sigismember(current->notifier_mask, sig)) {
> -				if (!(current->notifier)(current->notifier_data)) {
> -					clear_thread_flag(TIF_SIGPENDING);
> -					return 0;
> -				}
> +	if (unlikely(!sig))
> +		return 0;
> +
> +	list_for_each_entry_safe(notifier, tmp, &current->notifier_list, list) {
> +		if (sigismember(&notifier->mask, sig)) {
> +			if (!(notifier->func)(notifier->data)) {

	Normal kernel stile would be

			if (!notifier->func(notifier->data)) {


The function to add/remove the notifier should grow kerneldoc comments
while you're at it, and maybe more descriptive names.

Also the patch does an actual functional change because it allows for
multiple clients to register the notification, which is probably useful.

Re: [PATCH] shrink task_struct by 16 bytes

[Davi Arnaut]

Christoph Hellwig wrote:
> On Sun, May 20, 2007 at 12:40:11AM -0300, Davi Arnaut wrote:
>> Hi,
>>
>> Shrink task_struct by replacing the notifier callback with a
>> notifier list. The block_all_signals() function (and the signal
>> notifier mechanism) has only one user at the moment, which is drm.
> 
> This looks like a nice improvement.  Some comments below:
> 
>> -	int (*notifier)(void *priv);
>> -	void *notifier_data;
>> -	sigset_t *notifier_mask;
>> -	
>> +	/* signal notifier list */
>> +	struct list_head notifier_list;
>> +
> 
> This filed should have a more descripvtive name, e.g. singal_notifier_list.
> Also it's probably fine to use a hlist to save another pointer in
> struct task_struct.

Good catch.

>> +	struct signal_notifier *tmp, *notifier;
>>  	int sig = next_signal(pending, mask);
>>  
>> -	if (sig) {
>> -		if (current->notifier) {
>> -			if (sigismember(current->notifier_mask, sig)) {
>> -				if (!(current->notifier)(current->notifier_data)) {
>> -					clear_thread_flag(TIF_SIGPENDING);
>> -					return 0;
>> -				}
>> +	if (unlikely(!sig))
>> +		return 0;
>> +
>> +	list_for_each_entry_safe(notifier, tmp, &current->notifier_list, list) {
>> +		if (sigismember(&notifier->mask, sig)) {
>> +			if (!(notifier->func)(notifier->data)) {
> 
> 	Normal kernel stile would be
> 
> 			if (!notifier->func(notifier->data)) {
> 
> 
> The function to add/remove the notifier should grow kerneldoc comments
> while you're at it, and maybe more descriptive names.

Ok.

> Also the patch does an actual functional change because it allows for
> multiple clients to register the notification, which is probably useful.

Updated patch below:

Shrink task_struct by replacing the notifier callback with a notifier
list that allows for multiple clients to register notifiers. It also
saves one cacheline on i386.

The patch also renames the un/block_all_signals() functions to more
descriptive names signal_notifier_add/del() and updates the kerneldoc
comments accordingly. The only in-tree user (drm) is converted.

Thanks to Christoph Hellwig for useful comments and review.

Pahole output for task_struct:

i386 before:

   /* size: 2640, cachelines: 42 */
   /* sum members: 2619, holes: 3, sum holes: 9 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 12 */
   /* paddings: 1, sum paddings: 8 */
   /* last cacheline: 16 bytes */

i386 after:

   /* size: 2624, cachelines: 41 */
   /* sum members: 2611, holes: 3, sum holes: 9 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 4 */
   /* paddings: 1, sum paddings: 8 */

x86_64 before:

   /* size: 3632, cachelines: 57 */
   /* sum members: 3579, holes: 10, sum holes: 45 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 8 */
   /* last cacheline: 48 bytes */

x86_64 after:

   /* size: 3616, cachelines: 57 */
   /* sum members: 3563, holes: 10, sum holes: 45 */
   /* bit holes: 2, sum bit holes: 62 bits */
   /* padding: 8 */
   /* last cacheline: 32 bytes */

Signed-off-by: Davi E. M. Arnaut <davi@haxent.com.br>
Cc: Dave Airlie <airlied@linux.ie>
Cc: Christoph Hellwig <hch@infradead.org>

---
 drivers/char/drm/drmP.h     |    4 +-
 drivers/char/drm/drm_lock.c |   16 ++++++----
 include/linux/init_task.h   |    1 
 include/linux/sched.h       |   10 ++----
 include/linux/signal.h      |   21 ++++++++++++++
 kernel/fork.c               |    1 
 kernel/signal.c             |   66 ++++++++++++++++++++++----------------------
 7 files changed, 71 insertions(+), 48 deletions(-)

Index: linux-2.6/include/linux/sched.h
===================================================================
--- linux-2.6.orig/include/linux/sched.h
+++ linux-2.6/include/linux/sched.h
@@ -963,10 +963,9 @@ struct task_struct {
 
 	unsigned long sas_ss_sp;
 	size_t sas_ss_size;
-	int (*notifier)(void *priv);
-	void *notifier_data;
-	sigset_t *notifier_mask;
-	
+	/* struct signal_notifier callback list */
+	struct hlist_head signal_notifier_list;
+
 	void *security;
 	struct audit_context *audit_context;
 	seccomp_t seccomp;
@@ -1339,9 +1338,6 @@ static inline int dequeue_signal_lock(st
 	return ret;
 }	
 
-extern void block_all_signals(int (*notifier)(void *priv), void *priv,
-			      sigset_t *mask);
-extern void unblock_all_signals(void);
 extern void release_task(struct task_struct * p);
 extern int send_sig_info(int, struct siginfo *, struct task_struct *);
 extern int send_group_sig_info(int, struct siginfo *, struct task_struct *);
Index: linux-2.6/include/linux/init_task.h
===================================================================
--- linux-2.6.orig/include/linux/init_task.h
+++ linux-2.6/include/linux/init_task.h
@@ -157,6 +157,7 @@ extern struct group_info init_groups;
 		.list = LIST_HEAD_INIT(tsk.pending.list),		\
 		.signal = {{0}}},					\
 	.blocked	= {{0}},					\
+	.signal_notifier_list = HLIST_HEAD_INIT,			\
 	.alloc_lock	= __SPIN_LOCK_UNLOCKED(tsk.alloc_lock),		\
 	.journal_info	= NULL,						\
 	.cpu_timers	= INIT_CPU_TIMERS(tsk.cpu_timers),		\
Index: linux-2.6/include/linux/signal.h
===================================================================
--- linux-2.6.orig/include/linux/signal.h
+++ linux-2.6/include/linux/signal.h
@@ -27,6 +27,27 @@ struct sigpending {
 	sigset_t signal;
 };
 
+/**
+ * struct signal_notifier - the signal notifier struct
+ * @node: hash list node to enqueue into the process notifier list
+ * @func: notifier callback function. if the callback routine returns 0
+ * then then signal will be blocked.
+ * @data: pointer to private data that the notifier routine can use to
+ * determine if the signal should be blocked or not.
+ * @mask: mask of signals to call the callback routine upon
+ */
+struct signal_notifier {
+	struct hlist_node node;
+	int (*func)(void *priv);
+	void *data;
+	sigset_t mask;
+};
+
+/* Add a signal notifier struct to the current process notifier list  */
+extern void signal_notifier_add(struct signal_notifier *);
+/* Remove a signal notifier struct from the current process notifier list */
+extern void signal_notifier_del(struct signal_notifier *);
+
 /*
  * Define some primitives to manipulate sigset_t.
  */
Index: linux-2.6/kernel/fork.c
===================================================================
--- linux-2.6.orig/kernel/fork.c
+++ linux-2.6/kernel/fork.c
@@ -1034,6 +1034,7 @@ static struct task_struct *copy_process(
 	p->vfork_done = NULL;
 	spin_lock_init(&p->alloc_lock);
 
+	INIT_HLIST_HEAD(&p->signal_notifier_list);
 	clear_tsk_thread_flag(p, TIF_SIGPENDING);
 	init_sigpending(&p->pending);
 
Index: linux-2.6/drivers/char/drm/drm_lock.c
===================================================================
--- linux-2.6.orig/drivers/char/drm/drm_lock.c
+++ linux-2.6/drivers/char/drm/drm_lock.c
@@ -110,14 +110,16 @@ int drm_lock(struct inode *inode, struct
 	DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
 	if (ret) return ret;
 
-	sigemptyset(&dev->sigmask);
-	sigaddset(&dev->sigmask, SIGSTOP);
-	sigaddset(&dev->sigmask, SIGTSTP);
-	sigaddset(&dev->sigmask, SIGTTIN);
-	sigaddset(&dev->sigmask, SIGTTOU);
+	sigemptyset(&dev->notifier.mask);
+	sigaddset(&dev->notifier.mask, SIGSTOP);
+	sigaddset(&dev->notifier.mask, SIGTSTP);
+	sigaddset(&dev->notifier.mask, SIGTTIN);
+	sigaddset(&dev->notifier.mask, SIGTTOU);
 	dev->sigdata.context = lock.context;
 	dev->sigdata.lock = dev->lock.hw_lock;
-	block_all_signals(drm_notifier, &dev->sigdata, &dev->sigmask);
+	dev->notifier.data = &dev->sigdata;
+	dev->notifier.func = drm_notifier;
+	signal_notifier_add(&dev->notifier);
 
 	if (dev->driver->dma_ready && (lock.flags & _DRM_LOCK_READY))
 		dev->driver->dma_ready(dev);
@@ -189,7 +191,7 @@ int drm_unlock(struct inode *inode, stru
 		}
 	}
 
-	unblock_all_signals();
+	signal_notifier_del(&dev->notifier);
 	return 0;
 }
 
Index: linux-2.6/drivers/char/drm/drmP.h
===================================================================
--- linux-2.6.orig/drivers/char/drm/drmP.h
+++ linux-2.6/drivers/char/drm/drmP.h
@@ -750,8 +750,8 @@ typedef struct drm_device {
 	drm_sg_mem_t *sg;	/**< Scatter gather memory */
 	unsigned long *ctx_bitmap;	/**< context bitmap */
 	void *dev_private;		/**< device private data */
-	drm_sigdata_t sigdata;	   /**< For block_all_signals */
-	sigset_t sigmask;
+	drm_sigdata_t sigdata;	   /**< Signal notifier data */
+	struct signal_notifier notifier;	/**< For block_signals */
 
 	struct drm_driver *driver;
 	drm_local_map_t *agp_buffer_map;
Index: linux-2.6/kernel/signal.c
===================================================================
--- linux-2.6.orig/kernel/signal.c
+++ linux-2.6/kernel/signal.c
@@ -237,37 +237,35 @@ flush_signal_handlers(struct task_struct
 	}
 }
 
-
-/* Notify the system that a driver wants to block all signals for this
- * process, and wants to be notified if any signals at all were to be
- * sent/acted upon.  If the notifier routine returns non-zero, then the
- * signal will be acted upon after all.  If the notifier routine returns 0,
- * then then signal will be blocked.  Only one block per process is
- * allowed.  priv is a pointer to private data that the notifier routine
- * can use to determine if the signal should be blocked or not.  */
-
-void
-block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
+/**
+ * signal_notifier_add - Add a signal notifier callback to the current process
+ * @notifier: the signal_notifier struct to add
+ *
+ * Notify the system that a driver wants to block a set of signals for this
+ * process and wants to be notified if any signals at all were to be sent or
+ * acted upon. If the notifier routine returns non-zero, then the signal will
+ * be acted upon after all. If the notifier routine returns 0, the signal will
+ * be blocked.
+ */
+void signal_notifier_add(struct signal_notifier *notifier)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier_mask = mask;
-	current->notifier_data = priv;
-	current->notifier = notifier;
+	hlist_add_head(&notifier->node, &current->signal_notifier_list);
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 }
 
-/* Notify the system that blocking has ended. */
-
-void
-unblock_all_signals(void)
+/**
+ * signal_notifier_del - Notify the system that blocking has ended
+ * @notifier: the signal_notifier struct to remove
+ */
+void signal_notifier_del(struct signal_notifier *notifier)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&current->sighand->siglock, flags);
-	current->notifier = NULL;
-	current->notifier_data = NULL;
+	hlist_del(&notifier->node);
 	recalc_sigpending();
 	spin_unlock_irqrestore(&current->sighand->siglock, flags);
 }
@@ -318,22 +316,26 @@ static int collect_signal(int sig, struc
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
 			siginfo_t *info)
 {
+	struct hlist_node *entry, *next;
+	struct signal_notifier *notifier;
 	int sig = next_signal(pending, mask);
+	struct hlist_head *hash_list = &current->signal_notifier_list;
 
-	if (sig) {
-		if (current->notifier) {
-			if (sigismember(current->notifier_mask, sig)) {
-				if (!(current->notifier)(current->notifier_data)) {
-					clear_thread_flag(TIF_SIGPENDING);
-					return 0;
-				}
+	if (unlikely(!sig))
+		return 0;
+
+	hlist_for_each_entry_safe(notifier, entry, next, hash_list, node) {
+		if (sigismember(&notifier->mask, sig)) {
+			if (!notifier->func(notifier->data)) {
+				clear_thread_flag(TIF_SIGPENDING);
+				return 0;
 			}
 		}
-
-		if (!collect_signal(sig, pending, info))
-			sig = 0;
 	}
 
+	if (!collect_signal(sig, pending, info))
+		sig = 0;
+
 	return sig;
 }
 
@@ -1860,8 +1862,8 @@ EXPORT_SYMBOL(ptrace_notify);
 EXPORT_SYMBOL(send_sig);
 EXPORT_SYMBOL(send_sig_info);
 EXPORT_SYMBOL(sigprocmask);
-EXPORT_SYMBOL(block_all_signals);
-EXPORT_SYMBOL(unblock_all_signals);
+EXPORT_SYMBOL(signal_notifier_add);
+EXPORT_SYMBOL(signal_notifier_del);
 
 
 /*

Re: [PATCH] shrink task_struct by 16 bytes

[Christoph Hellwig]

Looks good.

Re: [PATCH] shrink task_struct by 16 bytes

[Dave Airlie]

>
> Shrink task_struct by replacing the notifier callback with a notifier
> list that allows for multiple clients to register notifiers. It also
> saves one cacheline on i386.
>
> The patch also renames the un/block_all_signals() functions to more
> descriptive names signal_notifier_add/del() and updates the kerneldoc
> comments accordingly. The only in-tree user (drm) is converted.
>
> Thanks to Christoph Hellwig for useful comments and review.
>
> Pahole output for task_struct:
>
> i386 before:
>
>    /* size: 2640, cachelines: 42 */
>    /* sum members: 2619, holes: 3, sum holes: 9 */
>    /* bit holes: 2, sum bit holes: 62 bits */
>    /* padding: 12 */
>    /* paddings: 1, sum paddings: 8 */
>    /* last cacheline: 16 bytes */
>
> i386 after:
>
>    /* size: 2624, cachelines: 41 */
>    /* sum members: 2611, holes: 3, sum holes: 9 */
>    /* bit holes: 2, sum bit holes: 62 bits */
>    /* padding: 4 */
>    /* paddings: 1, sum paddings: 8 */
>
> x86_64 before:
>
>    /* size: 3632, cachelines: 57 */
>    /* sum members: 3579, holes: 10, sum holes: 45 */
>    /* bit holes: 2, sum bit holes: 62 bits */
>    /* padding: 8 */
>    /* last cacheline: 48 bytes */
>
> x86_64 after:
>
>    /* size: 3616, cachelines: 57 */
>    /* sum members: 3563, holes: 10, sum holes: 45 */
>    /* bit holes: 2, sum bit holes: 62 bits */
>    /* padding: 8 */
>    /* last cacheline: 32 bytes */
>
> Signed-off-by: Davi E. M. Arnaut <davi@haxent.com.br>
> Cc: Dave Airlie <airlied@linux.ie>

Signed-off-by: Dave Airlie <airlied@linux.ie>

No problems from me either..

Dave.

> Cc: Christoph Hellwig <hch@infradead.org>
>
> ---
>  drivers/char/drm/drmP.h     |    4 +-
>  drivers/char/drm/drm_lock.c |   16 ++++++----
>  include/linux/init_task.h   |    1
>  include/linux/sched.h       |   10 ++----
>  include/linux/signal.h      |   21 ++++++++++++++
>  kernel/fork.c               |    1
>  kernel/signal.c             |   66 ++++++++++++++++++++++----------------------
>  7 files changed, 71 insertions(+), 48 deletions(-)
>
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -963,10 +963,9 @@ struct task_struct {
>
>         unsigned long sas_ss_sp;
>         size_t sas_ss_size;
> -       int (*notifier)(void *priv);
> -       void *notifier_data;
> -       sigset_t *notifier_mask;
> -
> +       /* struct signal_notifier callback list */
> +       struct hlist_head signal_notifier_list;
> +
>         void *security;
>         struct audit_context *audit_context;
>         seccomp_t seccomp;
> @@ -1339,9 +1338,6 @@ static inline int dequeue_signal_lock(st
>         return ret;
>  }
>
> -extern void block_all_signals(int (*notifier)(void *priv), void *priv,
> -                             sigset_t *mask);
> -extern void unblock_all_signals(void);
>  extern void release_task(struct task_struct * p);
>  extern int send_sig_info(int, struct siginfo *, struct task_struct *);
>  extern int send_group_sig_info(int, struct siginfo *, struct task_struct *);
> Index: linux-2.6/include/linux/init_task.h
> ===================================================================
> --- linux-2.6.orig/include/linux/init_task.h
> +++ linux-2.6/include/linux/init_task.h
> @@ -157,6 +157,7 @@ extern struct group_info init_groups;
>                 .list = LIST_HEAD_INIT(tsk.pending.list),               \
>                 .signal = {{0}}},                                       \
>         .blocked        = {{0}},                                        \
> +       .signal_notifier_list = HLIST_HEAD_INIT,                        \
>         .alloc_lock     = __SPIN_LOCK_UNLOCKED(tsk.alloc_lock),         \
>         .journal_info   = NULL,                                         \
>         .cpu_timers     = INIT_CPU_TIMERS(tsk.cpu_timers),              \
> Index: linux-2.6/include/linux/signal.h
> ===================================================================
> --- linux-2.6.orig/include/linux/signal.h
> +++ linux-2.6/include/linux/signal.h
> @@ -27,6 +27,27 @@ struct sigpending {
>         sigset_t signal;
>  };
>
> +/**
> + * struct signal_notifier - the signal notifier struct
> + * @node: hash list node to enqueue into the process notifier list
> + * @func: notifier callback function. if the callback routine returns 0
> + * then then signal will be blocked.
> + * @data: pointer to private data that the notifier routine can use to
> + * determine if the signal should be blocked or not.
> + * @mask: mask of signals to call the callback routine upon
> + */
> +struct signal_notifier {
> +       struct hlist_node node;
> +       int (*func)(void *priv);
> +       void *data;
> +       sigset_t mask;
> +};
> +
> +/* Add a signal notifier struct to the current process notifier list  */
> +extern void signal_notifier_add(struct signal_notifier *);
> +/* Remove a signal notifier struct from the current process notifier list */
> +extern void signal_notifier_del(struct signal_notifier *);
> +
>  /*
>   * Define some primitives to manipulate sigset_t.
>   */
> Index: linux-2.6/kernel/fork.c
> ===================================================================
> --- linux-2.6.orig/kernel/fork.c
> +++ linux-2.6/kernel/fork.c
> @@ -1034,6 +1034,7 @@ static struct task_struct *copy_process(
>         p->vfork_done = NULL;
>         spin_lock_init(&p->alloc_lock);
>
> +       INIT_HLIST_HEAD(&p->signal_notifier_list);
>         clear_tsk_thread_flag(p, TIF_SIGPENDING);
>         init_sigpending(&p->pending);
>
> Index: linux-2.6/drivers/char/drm/drm_lock.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/drm/drm_lock.c
> +++ linux-2.6/drivers/char/drm/drm_lock.c
> @@ -110,14 +110,16 @@ int drm_lock(struct inode *inode, struct
>         DRM_DEBUG( "%d %s\n", lock.context, ret ? "interrupted" : "has lock" );
>         if (ret) return ret;
>
> -       sigemptyset(&dev->sigmask);
> -       sigaddset(&dev->sigmask, SIGSTOP);
> -       sigaddset(&dev->sigmask, SIGTSTP);
> -       sigaddset(&dev->sigmask, SIGTTIN);
> -       sigaddset(&dev->sigmask, SIGTTOU);
> +       sigemptyset(&dev->notifier.mask);
> +       sigaddset(&dev->notifier.mask, SIGSTOP);
> +       sigaddset(&dev->notifier.mask, SIGTSTP);
> +       sigaddset(&dev->notifier.mask, SIGTTIN);
> +       sigaddset(&dev->notifier.mask, SIGTTOU);
>         dev->sigdata.context = lock.context;
>         dev->sigdata.lock = dev->lock.hw_lock;
> -       block_all_signals(drm_notifier, &dev->sigdata, &dev->sigmask);
> +       dev->notifier.data = &dev->sigdata;
> +       dev->notifier.func = drm_notifier;
> +       signal_notifier_add(&dev->notifier);
>
>         if (dev->driver->dma_ready && (lock.flags & _DRM_LOCK_READY))
>                 dev->driver->dma_ready(dev);
> @@ -189,7 +191,7 @@ int drm_unlock(struct inode *inode, stru
>                 }
>         }
>
> -       unblock_all_signals();
> +       signal_notifier_del(&dev->notifier);
>         return 0;
>  }
>
> Index: linux-2.6/drivers/char/drm/drmP.h
> ===================================================================
> --- linux-2.6.orig/drivers/char/drm/drmP.h
> +++ linux-2.6/drivers/char/drm/drmP.h
> @@ -750,8 +750,8 @@ typedef struct drm_device {
>         drm_sg_mem_t *sg;       /**< Scatter gather memory */
>         unsigned long *ctx_bitmap;      /**< context bitmap */
>         void *dev_private;              /**< device private data */
> -       drm_sigdata_t sigdata;     /**< For block_all_signals */
> -       sigset_t sigmask;
> +       drm_sigdata_t sigdata;     /**< Signal notifier data */
> +       struct signal_notifier notifier;        /**< For block_signals */
>
>         struct drm_driver *driver;
>         drm_local_map_t *agp_buffer_map;
> Index: linux-2.6/kernel/signal.c
> ===================================================================
> --- linux-2.6.orig/kernel/signal.c
> +++ linux-2.6/kernel/signal.c
> @@ -237,37 +237,35 @@ flush_signal_handlers(struct task_struct
>         }
>  }
>
> -
> -/* Notify the system that a driver wants to block all signals for this
> - * process, and wants to be notified if any signals at all were to be
> - * sent/acted upon.  If the notifier routine returns non-zero, then the
> - * signal will be acted upon after all.  If the notifier routine returns 0,
> - * then then signal will be blocked.  Only one block per process is
> - * allowed.  priv is a pointer to private data that the notifier routine
> - * can use to determine if the signal should be blocked or not.  */
> -
> -void
> -block_all_signals(int (*notifier)(void *priv), void *priv, sigset_t *mask)
> +/**
> + * signal_notifier_add - Add a signal notifier callback to the current process
> + * @notifier: the signal_notifier struct to add
> + *
> + * Notify the system that a driver wants to block a set of signals for this
> + * process and wants to be notified if any signals at all were to be sent or
> + * acted upon. If the notifier routine returns non-zero, then the signal will
> + * be acted upon after all. If the notifier routine returns 0, the signal will
> + * be blocked.
> + */
> +void signal_notifier_add(struct signal_notifier *notifier)
>  {
>         unsigned long flags;
>
>         spin_lock_irqsave(&current->sighand->siglock, flags);
> -       current->notifier_mask = mask;
> -       current->notifier_data = priv;
> -       current->notifier = notifier;
> +       hlist_add_head(&notifier->node, &current->signal_notifier_list);
>         spin_unlock_irqrestore(&current->sighand->siglock, flags);
>  }
>
> -/* Notify the system that blocking has ended. */
> -
> -void
> -unblock_all_signals(void)
> +/**
> + * signal_notifier_del - Notify the system that blocking has ended
> + * @notifier: the signal_notifier struct to remove
> + */
> +void signal_notifier_del(struct signal_notifier *notifier)
>  {
>         unsigned long flags;
>
>         spin_lock_irqsave(&current->sighand->siglock, flags);
> -       current->notifier = NULL;
> -       current->notifier_data = NULL;
> +       hlist_del(&notifier->node);
>         recalc_sigpending();
>         spin_unlock_irqrestore(&current->sighand->siglock, flags);
>  }
> @@ -318,22 +316,26 @@ static int collect_signal(int sig, struc
>  static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>                         siginfo_t *info)
>  {
> +       struct hlist_node *entry, *next;
> +       struct signal_notifier *notifier;
>         int sig = next_signal(pending, mask);
> +       struct hlist_head *hash_list = &current->signal_notifier_list;
>
> -       if (sig) {
> -               if (current->notifier) {
> -                       if (sigismember(current->notifier_mask, sig)) {
> -                               if (!(current->notifier)(current->notifier_data)) {
> -                                       clear_thread_flag(TIF_SIGPENDING);
> -                                       return 0;
> -                               }
> +       if (unlikely(!sig))
> +               return 0;
> +
> +       hlist_for_each_entry_safe(notifier, entry, next, hash_list, node) {
> +               if (sigismember(&notifier->mask, sig)) {
> +                       if (!notifier->func(notifier->data)) {
> +                               clear_thread_flag(TIF_SIGPENDING);
> +                               return 0;
>                         }
>                 }
> -
> -               if (!collect_signal(sig, pending, info))
> -                       sig = 0;
>         }
>
> +       if (!collect_signal(sig, pending, info))
> +               sig = 0;
> +
>         return sig;
>  }
>
> @@ -1860,8 +1862,8 @@ EXPORT_SYMBOL(ptrace_notify);
>  EXPORT_SYMBOL(send_sig);
>  EXPORT_SYMBOL(send_sig_info);
>  EXPORT_SYMBOL(sigprocmask);
> -EXPORT_SYMBOL(block_all_signals);
> -EXPORT_SYMBOL(unblock_all_signals);
> +EXPORT_SYMBOL(signal_notifier_add);
> +EXPORT_SYMBOL(signal_notifier_del);
>
>
>  /*
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>