[patch 2/2] Re: PAGG Threading Issues
Kingsley Cheung <[email protected]>
| Newsgroups | gmane.linux.process-aggregates |
|---|---|
| Message-ID | <[email protected]> |
Hi Erik, This is the second of the patches. I believe it is in the best interest of PAGG to be remain as container for Linux tasks and to let PAGG clients decide whether they are interested in thread groups (equivalent to processes) or individual threads. To achieve this we need to ensure we only attach individual tasks _after_ their thread group has been established in the fork code. For this reason this patch moves the fork attach to the end of copy_process(). Since this change can mean that we race with the registration procedure, the attach code also checks to ensure that tasks are not attached twice. Another change the patch makes is to continue attachments for other clients in pagg_attach even if the attach callback fails for one client. IMHO each client should be permitted to operate independently of each other. On Tue, Nov 09, 2004 at 10:00:39AM +1100, Kingsley Cheung wrote: > 2) Is pagg intended to be a container for Linux tasks or thread > groups? I'm avoiding the use of the word "process" as that could be > confusing in this context. > > The reason I'm asking is because throughout the pagg registration code > the for_each_process() macro is used. This happens in > remove_client_paggs_from_all_tasks() and pagg_hook_register(). The > problem is, however, since 2.6 for_each_process() only traverses > thread group leader tasks. Thus the registration and deregistration > code only considers thread group leaders. > > In contrast the pagg_attach() and pagg_detach() hooks are placed to > catch all tasks in the fork and exit code. With the way the > registration and deregistration code is behaving, this means that we > won't catch all existing tasks and we won't clean up after all tasks > properly for multi-threaded applications. > > So what is pagg intended for? To catch every single task? In that > case the do_each_thread() and while_each_thread() macros should be > used. Or is pagg a container for thread groups only? The changes for > that would require much more thought... Thanks, -- Kingsley
linux-2.6.9-pagg-attach.patch
(text/plain, 3.6 KB)
--- linux-2.6.5-7.97ZP2RC3/kernel/fork.c 2004-12-03 10:37:15.000000000 +1100
+++ linux-2.6.5-7.97ZP2RC4/kernel/fork.c 2004-12-08 16:59:26.480431272 +1100
@@ -1051,12 +1051,6 @@
sched_fork(p);
/*
- * call pagg modules to properly attach new process to the same
- * process aggregate containers as the parent process.
- */
- pagg_attach(p, current);
-
- /*
* Ok, make it visible to the rest of the system.
* We dont wake it up yet.
*/
@@ -1131,13 +1125,20 @@
write_unlock_irq(&tasklist_lock);
retval = 0;
+ /*
+ * call pagg modules to properly attach new process to the
+ * same process aggregate containers as the parent process.
+ * We do this after its thread group has been assigned to
+ * permit clients to distinguish thread groups clearly.
+ */
+ pagg_attach(p, current);
+
fork_out:
if (retval)
return ERR_PTR(retval);
return p;
bad_fork_cleanup_namespace:
- pagg_detach(p);
exit_namespace(p);
bad_fork_cleanup_mm:
exit_mm(p);
--- linux-2.6.5-7.97ZP2RC3/kernel/pagg.c 2004-12-03 10:37:15.000000000 +1100
+++ linux-2.6.5-7.97ZP2RC4/kernel/pagg.c 2004-12-08 17:04:08.512555872 +1100
@@ -261,7 +261,7 @@
struct task_struct *g = NULL, *p = NULL;
int init_result = 0;
- /* Because of internal race conditions we can't gaurantee
+ /* Because of internal race conditions we can't guarantee
* getting every task in just one pass so we just keep going
* until we don't find any unitialized tasks. The inefficiency
* of this should be tempered by the fact that this happens
@@ -389,32 +389,33 @@
{
struct pagg *from_pagg;
- /* lock the parents pagg_list we are copying from */
- down_read(&from_task->pagg_sem); /* read lock the pagg list */
+ /* Lock the parents pagg_list we are copying from. We will be
+ * on the task list already and may have competition with
+ * registering clients, so lock our own list.
+ */
+ down_read(&from_task->pagg_sem);
+ down_write(&to_task->pagg_sem);
list_for_each_entry(from_pagg, &from_task->pagg_list, entry) {
struct pagg *to_pagg = NULL;
- to_pagg = pagg_alloc(to_task, from_pagg->hook);
- if (!to_pagg) {
- goto error_return;
- }
- if (to_pagg->hook->attach(to_task, to_pagg, from_pagg->data) != 0 )
- goto error_return;
- }
-
- up_read(&from_task->pagg_sem); /* unlock the pagg list */
+ /* Raced with allocation from registration. */
+ if (pagg_get(to_task, from_pagg->hook->name) != NULL)
+ continue;
+
+ /* Break on memory allocation failure. */
+ if (!(to_pagg = pagg_alloc(to_task, from_pagg->hook)))
+ break;
- return; /* success */
+ /* Do not detach all attachments on error. This
+ * behaviour is inconsistent with registration.
+ */
+ if (to_pagg->hook->attach(to_task, to_pagg, from_pagg->data))
+ pagg_free(to_pagg);
+ }
- error_return:
- /*
- * Clean up all the pagg attachments made on behalf of the new
- * task. Set new task pagg ptr to NULL for return.
- */
+ up_write(&to_task->pagg_sem);
up_read(&from_task->pagg_sem); /* unlock the pagg list */
- __pagg_detach(to_task);
- return; /* failure */
}
/**
@@ -437,9 +438,9 @@
pagg->hook->detach(task, pagg);
pagg_free(pagg);
}
-
+
up_write(&task->pagg_sem); /* write unlock the pagg list */
-
+
return; /* 0 = success, else return last code for failure */
}
@@ -459,7 +460,6 @@
{
struct pagg *pagg;
- /* lock the parents pagg_list we are copying from */
down_read(&task->pagg_sem); /* lock the pagg list */
list_for_each_entry(pagg, &task->pagg_list, entry) {