Skip to contentSkip to conversation

Cleanup the new thread's creation

RFC PATCH In review Pavel Emelyanov 2 participants 3 messages Aug 24, 2007 – Aug 27, 2007 2 messages not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 Cleanup the new thread's creation

The major differences of creating a new thread from creating a new process is that

1. newbie's tgid is set to leader's
2. newbie's leader is set to leader
3. newbie is added to leader's thread_list

So move the initialization of these in one place. This helps in pid/tgid fields isolation.

---

kernel/fork.c +0 -0
index 7517efe..0b282a8 100644
@@ -950,6 +950,20 @@ static inline void rt_mutex_init_task(st

#endif
}

+static void setup_new_thread(struct task_struct *thr, struct task_struct *leader) +{
+ thr->tgid = leader->tgid;
+ thr->group_leader = leader;
+ list_add_tail_rcu(&thr->thread_group, &leader->thread_group);
+}
+
+static void setup_new_leader(struct task_struct *tsk)
+{
+ tsk->tgid = task_pid_nr(tsk);
+ tsk->group_leader = tsk;
+ INIT_LIST_HEAD(&tsk->thread_group);
+}
+
/*
* This creates a new process as a copy of the old one,
* but does not actually start it yet.
@@ -1147,9 +1161,6 @@ static struct task_struct *copy_process(

	}

	p->pid = pid_nr(pid);

- p->tgid = p->pid;
- if (clone_flags & CLONE_THREAD)
- p->tgid = current->tgid;

	p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL;
	/*

@@ -1191,8 +1202,6 @@ static struct task_struct *copy_process(
* Ok, make it visible to the rest of the system.
* We dont wake it up yet.
*/
- p->group_leader = p;

-	INIT_LIST_HEAD(&p->thread_group);
	INIT_LIST_HEAD(&p->ptrace_children);
	INIT_LIST_HEAD(&p->ptrace_list);

@@ -1251,8 +1260,7 @@ static struct task_struct *copy_process(

	}

	if (clone_flags & CLONE_THREAD) {

- p->group_leader = current->group_leader;
- list_add_tail_rcu(&p->thread_group, &p->group_leader->thread_group);
+ setup_new_thread(p, current->group_leader);

		if (!cputime_eq(current->signal->it_virt_expires,
				cputime_zero) ||
@@ -1268,7 +1276,8 @@ static struct task_struct *copy_process(
			 */
			p->it_prof_expires = jiffies_to_cputime(1);
		}

- }
+ } else
+ setup_new_leader(p);

	if (likely(p->pid)) {
		add_parent(p);

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 7 lines · Pavel Emelyanov

The major differences of creating a new thread from creating a new process is that

1. newbie's tgid is set to leader's
2. newbie's leader is set to leader
3. newbie is added to leader's thread_list

(Surely, the are many other major differences, but from the pids virtualization
POV - yes ;)

Review

Quoted 7 lines

+static void setup_new_thread(struct task_struct *thr, struct task_struct *leader)

(quoted diff) +5 -0
+{
+	thr->tgid = leader->tgid;
+	thr->group_leader = leader;
+	list_add_tail_rcu(&thr->thread_group, &leader->thread_group);
+}

Imho, this name is a bit "too generic". Not that I can suggest something better... copy_sub_thread/copy_group_leader ?

Review static struct task_struct *copy_process( · line 1152

Quoted 7 lines
(quoted diff) +0 -3
@@ -1147,9 +1161,6 @@ static struct task_struct *copy_process(
 	}
 
 	p->pid = pid_nr(pid);
-	p->tgid = p->pid;
-	if (clone_flags & CLONE_THREAD)
-		p->tgid = current->tgid;

I agree, it is absoulutely not clear why should we set ->tgid here, and it would be nice to consolidate "if (CLONE_THREAD)" checks, but do we really need the helpers above? There are very simple, and have the only one caller. Sometimes it is good to see what's going on without pressing C-]

Not that I against this patch, just I'm not sure it really simplifies things. Perhaps I missed something else you have in mind.

Oleg.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw
Quoted 35 lines · Oleg Nesterov
Quoted 6 lines · Pavel Emelyanov

The major differences of creating a new thread from creating a new process is that

1. newbie's tgid is set to leader's
2. newbie's leader is set to leader
3. newbie is added to leader's thread_list

(Surely, the are many other major differences, but from the pids virtualization
POV - yes ;)

Review

Quoted 7 lines

+static void setup_new_thread(struct task_struct *thr, struct task_struct *leader)

(quoted diff) +5 -0
+{
+	thr->tgid = leader->tgid;
+	thr->group_leader = leader;
+	list_add_tail_rcu(&thr->thread_group, &leader->thread_group);
+}

Imho, this name is a bit "too generic". Not that I can suggest something better... copy_sub_thread/copy_group_leader ?

Review static struct task_struct *copy_process( · line 1152

Quoted 7 lines
(quoted diff) +0 -3
@@ -1147,9 +1161,6 @@ static struct task_struct *copy_process(
 	}
 
 	p->pid = pid_nr(pid);
-	p->tgid = p->pid;
-	if (clone_flags & CLONE_THREAD)
-		p->tgid = current->tgid;

I agree, it is absoulutely not clear why should we set ->tgid here, and it would be nice to consolidate "if (CLONE_THREAD)" checks, but do we really need the helpers above? There are very simple, and have the only one caller. Sometimes it is good to see what's going on without pressing C-]

Quoted 2 lines

Not that I against this patch, just I'm not sure it really simplifies things. Perhaps I missed something else you have in mind.

Me too, but while cleaning up the pid_t usage over the kernel I found
this place to be one of the most difficult from "how to make it better"
point of view. We need to hide the pid/tgid explicit usage somehow, but the problem is that pid and tgid are set in this place and de_thread() only and making helpers like set_task_tgid() doesn't sound reasonable.

Quoted 3 lines

Oleg.

Thanks,
Pavel