Re: Task server thread and task allocation/deallocation interfaces proposal

[Matthieu Lemerre]

"Neal H. Walfield" <address@hidden> writes:

>> For basic operations we need the following operations:
>> 
>> ***task_create(hurd_cap_handle_t current_task_handle,
>>              hurd_cap_handle_t *new_task_handle)
>> 
>> This create a new task, with just one thread, and creates an handle
>> associated with it so you can act on the task.
>
> The first argument should be a task server root capability found in
> the hurd_startup_data.  As an alternative, we could use
> file_name_lookup ("/server/task").  This latter approach means that a
> task finds the task server based on its environment.  Having only this
> makes it difficult to force a child to use a different task server:
> the parent would have to create an entire chroot-like environment.
> This is inconvenient.  As I see no added flexibility, I think that the
> creator task should pass the task server to the childin the
> hurd_startup_data.

OK. I didn't thought about all this, I just forgot the thread id :).
I also forgot the kip address and the utcb fpage, so I propose:
task_create(l4_thread_id_t task_server,
            hurd_cap_handle_t current_task_handle,
            void *kip,
            l4_fpage_t utcb,
            hurd_cap_handle_t *new_task_handle)

I wonder if kip should be passed as a fpage or a void * (since the
size of the kip area is constant, base address is sufficient).


>
> Second, allocating a task should not allocate a thread.  Those are
>separate operations there is no need to artificially merge them at
>this level.  This is easy enough to do at the POSIX level.

There is some problems though: you cannot do a space_control operation
if there isn't any thread. So we would have to store the kip and utcb
parameters, and do the space control when the first thread is
allocated.

Space control could also fail during this first thread allocations, so
we would have to tell the client that "task creation failed", when for
it, task creation already took place and succeed. I find it more
confusing.

(I checked and in fact, we could maybe ensure that space control won't
fail by carefully checking the kip and utcb arguments in task_create).

So the implementation wouldn't be easier in fact.

>
>> Task creation requires the following steps:
>> -creating a new inactive thread
>> -calling space_control on it
>> -Activating the thread, that is to say setting a pager for this thread.
>> 
>> Setting the pager can be done:
>> -By a separate RPC, task_set_pager(hurd_cap_handle_t task_handle,
>>                                    l4_thread_id_t pager,
>>                                    l4_thread_id_t thread).
>
> This isn't needed if we set the pager when creating the thread.  Then
> it automatically starts waiting for a message from the pager.
>

Right, I did not express myself clearly. I was meaning that "We have
to choose one way to set the pager between these three different
methods", and modifying thread_alloc seems to be the best one.

>
>> ***task_terminate(hurd_cap_handle_t task_handle)
>> 
>> This RPC sends task death notification to each task which hold a task
>> info capability, and destroy all threads in the task, thus destroying
>> the address space.
>> It also record somewhere that the task_id is not used anymore.
>
> That looks fine.
>
>> *** task_thread_alloc(hurd_cap_handle_t task_handle,
>>                       l4_thread_id_t pager,
>>                       l4_word_t number_requested,
>>                       l4_word_t all_or_nothing,
>>                       l4_word_t *number_allocated,
>>                       l4_thread_id_t *thread_ids)
>> 
>> Neal wanted to alloc several threads at one time, so:
>> 
>> This allocates NUMBER threads, or less if the server cannot or does
>> not want (unless all_or_nothing is set, in which case no thread is
>> allocated). Every created thread has its pager set to PAGER.
>
> This looks good.  But rename all_or_nothing to flags and create a
> macro called TASK_THREAD_ALLOW_PARTIAL.  This way, we can add more
> flags later.
>

OK, I'll do this.  I also forgot the utcb location for the thread here.

>
>> *** task_thread_dealloc(hurd_cap_handle_t task_handle,
>>                         l4_word_t number,
>>                         l4_thread_id_t *thread_ids)
>> 
>> This destroy the threads and put the thread ids back in the free
>> thread ids list.
>
> Good.  But you need to know how many were successfully deallocated.
> If I pass 3 tids and the second is bad (but the first and third are
> valid) then only the first should be deallocated and an error, such as
> EINVAL, should be returned and the number of deallocations set to 1.

OK. So : task_thread_dealloc(hurd_cap_handle_t task_handle,
                             l4_word_t number_to_deallocate,
                             l4_thread_id_t *thread_ids,
                             l4_word_t *number_deallocated)

>
>> When there is only one thread left, the thread is not destroyed but
>> just inactivated.
>
> This is an interesting case.  I think it might be better to just
> implicitly deallocate the address space.

Yes, that would be coherent with "allocating a task should not
allocate a thread".

>
>> (And in thread_alloc, if there is nr_threads is 0,
>> the first thread is activated and the other allocated).
>
> What do you mean?
>

I meant this: when the client want to allocate the first thread, since
it is already created (in the scheme I exposed), we only need to
activate it, only the others are allocated.

>
>
>> *On the implementation side, I suppose that we should use the
>> task_alloc, ... functions used in the cap_class.
>
> Yes.
>

Then I'm wondering "What should go where":

-Where should thread_control/space_control operations happen: in
task_create or in the hurd_cap_class_alloc function? (This isn't
needed if we call space_control only after allocating the first
thread, but even in this case, I'd like to know).

-Same for task_terminate: I think that it does not destroy the threads
 itself, but let task_reinit do it? So it should only drop the
 reference to the task object?

>
>> So we should rename the task_alloc function currently written in
>> task_alloc_bootstrap or something like that.
>
> I am not clear about what you want to do here.

There is already a task_alloc function, and I thought it was nicer to
rename it task_alloc_bootstrap so that task_alloc is the function used
in the capability class.

>> 
>> -This one implements wortel_space_control in wortel:
>> 
>> +      else if (label == WORTEL_MSG_SPACE_CONTROL)
>> +    {
>> +      if (l4_untyped_words (tag) != 5 || l4_typed_words (tag) != 0)
>> +        panic ("Invalid format of thread control msg");
>
> I think you want to change the error message.

Oh, I didn't see that. Sorry, I must have reviewed my patch too
quickly.

Here's a corrected version of this patch:

2005-03-04  Matthieu Lemerre <address@hidden>

        * wortel.c (serve_bootstrap_requests): Implement reply to
        WORTEL_MSG_SPACE_CONTROL.
        
--- mainline/hurd-l4/wortel/wortel.c    2005-02-14 01:22:16.000000000 +0000
+++ hurd-l4/wortel/wortel.c     2005-02-27 22:54:37.000000000 +0000
@@ -1231,6 +1231,27 @@ serve_bootstrap_requests (void)
          l4_msg_load (msg);
          l4_reply (from);
        }
+      else if (label == WORTEL_MSG_SPACE_CONTROL)
+       {
+         if (l4_untyped_words (tag) != 5 || l4_typed_words (tag) != 0)
+           panic ("Invalid format of space control msg");
+
+         l4_thread_id_t space = l4_msg_word (msg, 0);
+         l4_word_t control = l4_msg_word (msg, 1);
+         l4_fpage_t kip = l4_msg_word (msg, 2);
+         l4_fpage_t utcb = l4_msg_word (msg, 3);
+         l4_thread_id_t redirector = l4_msg_word (msg, 4);
+         l4_word_t ret;
+         l4_word_t old_control;
+
+         ret = l4_space_control (space, control, kip, utcb,
+                                 redirector, &old_control);
+         l4_msg_clear (msg);
+         l4_msg_append_word (msg, ret ? 0 : l4_error_code ());
+         l4_msg_append_word (msg, old_control);
+         l4_msg_load (msg);
+         l4_reply (from);
+       }
       else if (label == WORTEL_MSG_GET_TASK_CAP_REQUEST)
        {
          if (cur_task == (unsigned int) -1)

>
> The rest of this patch looks fine as do the other 2 patches.  Marcus
>can check them in if he is happy with them.

Thanks,
Matthieu