Re: [PATCH] shrink task_struct by 16 bytes

[Christoph Hellwig <hch@infradead.org>]

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.
-
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/