Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Christoph Hellwig]

On Thu, Apr 13, 2006 at 08:37:27PM +0400, Oleg Nesterov wrote:
> Simpllify do_each_task_pid/while_each_task_pid macros.
> This also makes the code a bit smaller.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> --- MM/include/linux/pid.h~	2006-03-23 22:48:10.000000000 +0300
> +++ MM/include/linux/pid.h	2006-04-13 20:28:53.000000000 +0400
> @@ -99,21 +99,16 @@ extern void FASTCALL(free_pid(struct pid
>  			pids[(type)].node)
>  
>  
> -/* We could use hlist_for_each_entry_rcu here but it takes more arguments
> - * than the do_each_task_pid/while_each_task_pid.  So we roll our own
> - * to preserve the existing interface.
> - */
> -#define do_each_task_pid(who, type, task)				\
> -	if ((task = find_task_by_pid_type(type, who))) {		\
> -		prefetch(pid_next(task, type));				\
> -		do {
> -
> -#define while_each_task_pid(who, type, task)				\
> -		} while (pid_next(task, type) &&  ({			\
> -				task = pid_next_task(task, type);	\
> -				rcu_dereference(task);			\
> -				prefetch(pid_next(task, type));		\
> -				1; }) );				\
> -	}
> +#define do_each_task_pid(who, type, task)					\
> +	do {									\
> +		struct hlist_node *pos___;					\
> +		struct pid *pid___ = find_pid(who);				\
> +		if (pid___ != NULL)						\
> +			hlist_for_each_entry_rcu((task), pos___,		\
> +				&pid___->tasks[type], pids[type].node) {
> +
> +#define while_each_task_pid(who, type, task)					\
> +			}							\
> +	} while (0)

This is prtty ugly.  Can't we just have a

#define for_each_task_pid(task, pid, type, pos) \
	hlist_for_each_entry_rcu((task), (pos),  \
		(&(pid))->tasks[type], pids[type].node) {

and move the find_pid to the caller?  That would make the code a whole lot
more readable.

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Arjan van de Ven]

> Then the caller should check find_pid() doesn't return NULL. But yes,
> we can hide this check inside for_each_task_pid().
> 
> But what about current users of do_each_task_pid ? We can't just remove
> these macros.

you can if you fix the callers :)

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Christoph Hellwig]

On Thu, Apr 13, 2006 at 09:54:31PM +0400, Oleg Nesterov wrote:
> > 
> > #define for_each_task_pid(task, pid, type, pos) \
> > 	hlist_for_each_entry_rcu((task), (pos),  \
> > 		(&(pid))->tasks[type], pids[type].node) {
> > 
> > and move the find_pid to the caller?  That would make the code a whole lot
> > more readable.
> 
> Then the caller should check find_pid() doesn't return NULL. But yes,
> we can hide this check inside for_each_task_pid().
> 
> But what about current users of do_each_task_pid ? We can't just remove
> these macros.

They'd have to switch over to the new variant.  There's just 18 callers
ayway, currently, and with a patch like the one below that number firther
decreases :)


Index: linux-2.6/drivers/char/tty_io.c
===================================================================
--- linux-2.6.orig/drivers/char/tty_io.c	2006-04-13 16:22:12.000000000 +0200
+++ linux-2.6/drivers/char/tty_io.c	2006-04-13 16:39:57.000000000 +0200
@@ -1174,6 +1174,17 @@
 
 EXPORT_SYMBOL(tty_hung_up_p);
 
+static void clear_session_ttys(pid_t session)
+{
+	struct task_struct *p;
+
+	read_lock(&tasklist_lock);
+	do_each_task_pid(session, PIDTYPE_SID, p) {
+		p->signal->tty = NULL;
+	} while_each_task_pid(session, PIDTYPE_SID, p);
+	read_unlock(&tasklist_lock);
+}
+
 /*
  * This function is typically called only by the session leader, when
  * it wants to disassociate itself from its controlling tty.
@@ -1190,7 +1201,6 @@
 void disassociate_ctty(int on_exit)
 {
 	struct tty_struct *tty;
-	struct task_struct *p;
 	int tty_pgrp = -1;
 
 	lock_kernel();
@@ -1224,11 +1234,7 @@
 	tty->pgrp = -1;
 
 	/* Now clear signal->tty under the lock */
-	read_lock(&tasklist_lock);
-	do_each_task_pid(current->signal->session, PIDTYPE_SID, p) {
-		p->signal->tty = NULL;
-	} while_each_task_pid(current->signal->session, PIDTYPE_SID, p);
-	read_unlock(&tasklist_lock);
+	clear_session_ttys(current->signal->session);
 	mutex_unlock(&tty_mutex);
 	unlock_kernel();
 }
@@ -1927,17 +1933,9 @@
 	 * tty.
 	 */
 	if (tty_closing || o_tty_closing) {
-		struct task_struct *p;
-
-		read_lock(&tasklist_lock);
-		do_each_task_pid(tty->session, PIDTYPE_SID, p) {
-			p->signal->tty = NULL;
-		} while_each_task_pid(tty->session, PIDTYPE_SID, p);
+		clear_session_ttys(tty->session);
 		if (o_tty)
-			do_each_task_pid(o_tty->session, PIDTYPE_SID, p) {
-				p->signal->tty = NULL;
-			} while_each_task_pid(o_tty->session, PIDTYPE_SID, p);
-		read_unlock(&tasklist_lock);
+			clear_session_ttys(o_tty->session);
 	}
 
 	mutex_unlock(&tty_mutex);
@@ -2348,8 +2346,6 @@
 
 static int tiocsctty(struct tty_struct *tty, int arg)
 {
-	task_t *p;
-
 	if (current->signal->leader &&
 	    (current->signal->session == tty->session))
 		return 0;
@@ -2364,18 +2360,12 @@
 		 * This tty is already the controlling
 		 * tty for another session group!
 		 */
-		if ((arg == 1) && capable(CAP_SYS_ADMIN)) {
-			/*
-			 * Steal it away
-			 */
-
-			read_lock(&tasklist_lock);
-			do_each_task_pid(tty->session, PIDTYPE_SID, p) {
-				p->signal->tty = NULL;
-			} while_each_task_pid(tty->session, PIDTYPE_SID, p);
-			read_unlock(&tasklist_lock);
-		} else
+		if (arg != 1 || !capable(CAP_SYS_ADMIN))
 			return -EPERM;
+		/*
+		 * Steal it away
+		 */
+		clear_session_ttys(tty->session);
 	}
 	task_lock(current);
 	current->signal->tty = tty;

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Christoph Hellwig]

On Fri, Apr 14, 2006 at 12:21:04AM +0400, Oleg Nesterov wrote:
> #define NEW_IMPROVED_HLIST_FOR_EACH_ENTRY_RCU_WHICH_DOESNT_NEED_EXTRA_PARM(pos, head, member)	\
> 	for (pos = hlist_entry((head)->first, typeof(*(pos)), member);			\
> 		rcu_dereference(pos) != hlist_entry(NULL, typeof(*(pos)), member)	\
> 			&& ({ prefetch((pos)->member.next); 1; });			\
> 		(pos) = hlist_entry((pos)->member.next, typeof(*(pos)), member))
> 
> What do you think? What should be the name for it?

Justy kill the superflous argument from all hlist_for_each_entry variants
without a name change.

[PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Oleg Nesterov]

Simpllify do_each_task_pid/while_each_task_pid macros.
This also makes the code a bit smaller.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- MM/include/linux/pid.h~	2006-03-23 22:48:10.000000000 +0300
+++ MM/include/linux/pid.h	2006-04-13 20:28:53.000000000 +0400
@@ -99,21 +99,16 @@ extern void FASTCALL(free_pid(struct pid
 			pids[(type)].node)
 
 
-/* We could use hlist_for_each_entry_rcu here but it takes more arguments
- * than the do_each_task_pid/while_each_task_pid.  So we roll our own
- * to preserve the existing interface.
- */
-#define do_each_task_pid(who, type, task)				\
-	if ((task = find_task_by_pid_type(type, who))) {		\
-		prefetch(pid_next(task, type));				\
-		do {
-
-#define while_each_task_pid(who, type, task)				\
-		} while (pid_next(task, type) &&  ({			\
-				task = pid_next_task(task, type);	\
-				rcu_dereference(task);			\
-				prefetch(pid_next(task, type));		\
-				1; }) );				\
-	}
+#define do_each_task_pid(who, type, task)					\
+	do {									\
+		struct hlist_node *pos___;					\
+		struct pid *pid___ = find_pid(who);				\
+		if (pid___ != NULL)						\
+			hlist_for_each_entry_rcu((task), pos___,		\
+				&pid___->tasks[type], pids[type].node) {
+
+#define while_each_task_pid(who, type, task)					\
+			}							\
+	} while (0)
 
 #endif /* _LINUX_PID_H */

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Eric W. Biederman]

Christoph Hellwig <hch@infradead.org> writes:

> On Fri, Apr 14, 2006 at 12:21:04AM +0400, Oleg Nesterov wrote:
>> #define
> NEW_IMPROVED_HLIST_FOR_EACH_ENTRY_RCU_WHICH_DOESNT_NEED_EXTRA_PARM(pos, head,
> member) \
>> for (pos = hlist_entry((head)->first, typeof(*(pos)), member); \
>> rcu_dereference(pos) != hlist_entry(NULL, typeof(*(pos)), member) \
>> && ({ prefetch((pos)->member.next); 1; }); \
>> (pos) = hlist_entry((pos)->member.next, typeof(*(pos)), member))
>> 
>> What do you think? What should be the name for it?
>
> Justy kill the superflous argument from all hlist_for_each_entry variants
> without a name change.

Sounds like a plan.

I didn't attack this in the orignal patch that made it possible
because it would have all been noise there.

But as two independent cleanups it sounds reasonable.

Eric

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Oleg Nesterov]

On 04/13, Christoph Hellwig wrote:
>
> On Thu, Apr 13, 2006 at 08:37:27PM +0400, Oleg Nesterov wrote:
> > +#define do_each_task_pid(who, type, task)					\
> > +	do {									\
> > +		struct hlist_node *pos___;					\
> > +		struct pid *pid___ = find_pid(who);				\
> > +		if (pid___ != NULL)						\
> > +			hlist_for_each_entry_rcu((task), pos___,		\
> > +				&pid___->tasks[type], pids[type].node) {
> > +
> > +#define while_each_task_pid(who, type, task)					\
> > +			}							\
> > +	} while (0)
> 
> This is prtty ugly.  Can't we just have a
> 
> #define for_each_task_pid(task, pid, type, pos) \
> 	hlist_for_each_entry_rcu((task), (pos),  \
> 		(&(pid))->tasks[type], pids[type].node) {
> 
> and move the find_pid to the caller?  That would make the code a whole lot
> more readable.

Then the caller should check find_pid() doesn't return NULL. But yes,
we can hide this check inside for_each_task_pid().

But what about current users of do_each_task_pid ? We can't just remove
these macros.

Oleg.

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Oleg Nesterov]

On 04/13, Christoph Hellwig wrote:
>
> On Thu, Apr 13, 2006 at 09:54:31PM +0400, Oleg Nesterov wrote:
> > > 
> > > #define for_each_task_pid(task, pid, type, pos) \
> > > 	hlist_for_each_entry_rcu((task), (pos),  \
> > > 		(&(pid))->tasks[type], pids[type].node) {
> > > 
> > > and move the find_pid to the caller?  That would make the code a whole lot
> > > more readable.
> > 
> > Then the caller should check find_pid() doesn't return NULL. But yes,
> > we can hide this check inside for_each_task_pid().
> > 
> > But what about current users of do_each_task_pid ? We can't just remove
> > these macros.
> 
> They'd have to switch over to the new variant.  There's just 18 callers
> ayway, currently, and with a patch like the one below that number firther
> decreases :)

Ok, In such a case we should first

#define NEW_IMPROVED_HLIST_FOR_EACH_ENTRY_RCU_WHICH_DOESNT_NEED_EXTRA_PARM(pos, head, member)	\
	for (pos = hlist_entry((head)->first, typeof(*(pos)), member);			\
		rcu_dereference(pos) != hlist_entry(NULL, typeof(*(pos)), member)	\
			&& ({ prefetch((pos)->member.next); 1; });			\
		(pos) = hlist_entry((pos)->member.next, typeof(*(pos)), member))

What do you think? What should be the name for it?

Oleg.

Re: [PATCH] pids: simplify do_each_task_pid/while_each_task_pid

[Oleg Nesterov]

On 04/13, Christoph Hellwig wrote:
>
> This is prtty ugly.  Can't we just have a
> 
> #define for_each_task_pid(task, pid, type, pos) \
> 	hlist_for_each_entry_rcu((task), (pos),  \
> 		(&(pid))->tasks[type], pids[type].node) {
> 
> and move the find_pid to the caller?  That would make the code a whole lot
> more readable.

How about this? :)

Oleg.

--- MM/include/linux/pid.h~	2006-03-23 22:48:10.000000000 +0300
+++ MM/include/linux/pid.h	2006-04-14 01:49:08.000000000 +0400
@@ -98,22 +98,22 @@ extern void FASTCALL(free_pid(struct pid
 	hlist_entry(pid_next(task, type), struct task_struct,	\
 			pids[(type)].node)
 
+#define for_each_task_pid(who, type, t)		 							\
+	for ((t) = ({											\
+		struct pid *pid___ = find_pid(who);							\
+		pid___											\
+			? hlist_entry((&pid___->tasks[type])->first, typeof(*(t)), pids[type].node)	\
+			: hlist_entry(NULL, typeof(*(t)), pids[type].node);				\
+		});											\
+		rcu_dereference(t) != hlist_entry(NULL, typeof(*(t)), pids[type].node)			\
+			&& ({ prefetch((t)->pids[type].node.next); 1; });				\
+		(t) = hlist_entry((t)->pids[type].node.next, typeof(*(t)), pids[type].node))
 
-/* We could use hlist_for_each_entry_rcu here but it takes more arguments
- * than the do_each_task_pid/while_each_task_pid.  So we roll our own
- * to preserve the existing interface.
- */
-#define do_each_task_pid(who, type, task)				\
-	if ((task = find_task_by_pid_type(type, who))) {		\
-		prefetch(pid_next(task, type));				\
-		do {
+/* obsolete */
+#define do_each_task_pid(who, type, task)	\
+	do for_each_task_pid(who, type, task)
 
-#define while_each_task_pid(who, type, task)				\
-		} while (pid_next(task, type) &&  ({			\
-				task = pid_next_task(task, type);	\
-				rcu_dereference(task);			\
-				prefetch(pid_next(task, type));		\
-				1; }) );				\
-	}
+#define while_each_task_pid(who, type, task)	\
+	while (0)
 
 #endif /* _LINUX_PID_H */