[patch 1/2] Re: PAGG Threading Issues
Kingsley Cheung <[email protected]>
| Newsgroups | gmane.linux.process-aggregates |
|---|---|
| Message-ID | <[email protected]> |
Hi Erik,
I've had a chance to look at the issue and have come up with two
patches. This email has the first of two patches. This patch
addresses the first problem described below by using the "thread"
macros during registration and deregistration. It also makes a check
to skip exiting processes.
On Tue, Nov 09, 2004 at 10:00:39AM +1100, Kingsley Cheung wrote:
> Hi Erik,
>
> Some other issues as well. Please correct me if I'm wrong :)
>
> 1) I've noticed that pagg_detach() has been moved from
> __put_task_struct() to do_exit() between the pagg patches for Linux
> 2.6.8. This occurred as described at
> http://oss.sgi.com/archives/pagg/2004-08/msg00002.html
>
> Anyway, I think the move might have raised a problem with the
> registration code. Below is the relevant segment of code I'd like to
> consider:
>
> get_task_struct(p);
> read_unlock(&tasklist_lock);
> down_write(&p->pagg_sem);
> paggp = pagg_get(p, pagg_hook_new->name);
> if (paggp == NULL) {
> paggp = pagg_alloc(p, pagg_hook_new);
> if (paggp != NULL)
> init_result = pagg_hook_new->init(p, paggp);
> else
> init_result = -ENOMEM;
> }
> up_write(&p->pagg_sem);
> read_lock(&tasklist_lock);
> /* Like in remove_client_paggs_from_all_tasks, if the task
> * disappeared on us while we were going through the
> * for_each_process loop, we need to start over with that loop.
> * That's why we have the list_empty here */
> task_exited = list_empty(&p->tasks);
> put_task_struct(p);
>
> Now the tasks on the task list include those that are zombies. This
> occurs during exit_notify(), which sets their state to ZOMBIE but
> leaves them on the task list. Therefore the above code will allocate
> paggs for zombie tasks as well. [Note that task_exited won't trigger
> as the task is still on the task list.]
>
> Now unfortunately, by moving pagg_detach() out of __put_task_struct(),
> when the zombie has its task structure released, the pagg that was
> allocated during registration is no longer detached. Thus we'll could
> have a memory leak once the task structure is dropped and
> detach_pagg() is not invoked.
>
> In fact the leak could happen not just for zombies, but for tasks just
> about to exit. The race occurs in do_exit(), in which the
> detach_pagg() could be called before we allocate the pagg.
>
> Perhaps the quickest solution is to place a detach_pagg() call back
> into __put_task_struct(). Another suggestion might be to check for
> the PF_EXITING flag when allocating the pagg.
Thanks,
--
Kingsley
linux-2.6.9-pagg-threading.patch
(text/plain, 3.6 KB)
Index: linux/kernel/pagg.c
diff -u -r1.1.2.2 pagg.c
--- linux/kernel/pagg.c 15 Nov 2004 01:54:58 -0000 1.1.2.2
+++ linux/kernel/pagg.c 8 Dec 2004 06:25:22 -0000
@@ -172,11 +172,12 @@
* happens at most once for each registered client.
*/
while (atomic_read(&php->refcnt) != 0) {
- struct task_struct *p = NULL;
+ struct task_struct *g = NULL, *p = NULL;
read_lock(&tasklist_lock);
- for_each_process(p) {
+ do_each_thread(g, p) {
struct pagg *paggp;
+ int task_exited;
get_task_struct(p);
read_unlock(&tasklist_lock);
@@ -191,15 +192,14 @@
/* If a PAGG got removed from the list while we're going through
* each process, the tasks list for the process would be empty. In
- * that case, break out of this for_each_process so we can do it
+ * that case, break out of this for_each_thread so we can do it
* again. */
- if (list_empty(&p->tasks)) {
- put_task_struct(p);
- break;
- } else
- put_task_struct(p);
-
- }
+ task_exited = list_empty(&p->sibling);
+ put_task_struct(p);
+ if (task_exited)
+ goto endloop;
+ } while_each_thread(g, p);
+ endloop:
read_unlock(&tasklist_lock);
}
}
@@ -258,8 +258,8 @@
/* Now we can call the initializer function (if present) for each task */
if (pagg_hook_new->init != NULL) {
+ struct task_struct *g = NULL, *p = NULL;
int init_result = 0;
- int task_exited = 0;
/* Because of internal race conditions we can't gaurantee
* getting every task in just one pass so we just keep going
@@ -267,38 +267,38 @@
* of this should be tempered by the fact that this happens
* at most once for each registered client.
*/
- do {
- struct task_struct *p = NULL;
-
- read_lock(&tasklist_lock);
- for_each_process(p) {
- struct pagg *paggp;
+ read_lock(&tasklist_lock);
+ repeat:
+ do_each_thread(g, p) {
+ struct pagg *paggp;
+ int task_exited;
- get_task_struct(p);
- read_unlock(&tasklist_lock);
- down_write(&p->pagg_sem);
- paggp = pagg_get(p, pagg_hook_new->name);
- if (paggp == NULL) {
- paggp = pagg_alloc(p, pagg_hook_new);
- if (paggp != NULL)
- init_result = pagg_hook_new->init(p, paggp);
- else
- init_result = -ENOMEM;
- }
- up_write(&p->pagg_sem);
- read_lock(&tasklist_lock);
- /* Like in remove_client_paggs_from_all_tasks, if the task
- * disappeared on us while we were going through the
- * for_each_process loop, we need to start over with that loop.
- * That's why we have the list_empty here */
- task_exited = list_empty(&p->tasks);
- put_task_struct(p);
- if ((init_result != 0) || task_exited) {
- break;
- }
- }
+ get_task_struct(p);
read_unlock(&tasklist_lock);
- } while ((init_result == 0) && task_exited);
+ down_write(&p->pagg_sem);
+ paggp = pagg_get(p, pagg_hook_new->name);
+ if (!paggp && !(p->flags & PF_EXITING)) {
+ paggp = pagg_alloc(p, pagg_hook_new);
+ if (paggp != NULL)
+ init_result = pagg_hook_new->init(p, paggp);
+ else
+ init_result = -ENOMEM;
+ }
+ up_write(&p->pagg_sem);
+ read_lock(&tasklist_lock);
+ /* Like in remove_client_paggs_from_all_tasks, if the task
+ * disappeared on us while we were going through the
+ * for_each_thread loop, we need to start over with that loop.
+ * That's why we have the list_empty here */
+ task_exited = list_empty(&p->sibling);
+ put_task_struct(p);
+ if (init_result != 0)
+ goto endloop;
+ if (task_exited)
+ goto repeat;
+ } while_each_thread(g, p);
+ endloop:
+ read_unlock(&tasklist_lock);
/*
* if anything went wrong during initialisation abandon the