Kernel KAPI and instrumentation mods

[Jonathan Larmour]

Hi Nick,

I was looking over the patch while going through the backlog and was 
wondering about the following function. While the scheduler is locked 
inside the function, it obviously may well not be outside. In that case 
the thread being pointed to by current may have exitted and the memory (at 
least partially) reused since the last invocation of 
cyg_thread_get_next(). For example the unique id in the thread structure 
could be intact, but the list pointers scribbled on.

So I'm not sure this particular choice of implementation is wise. While we 
could insist on the scheduler being locked beforehand, that's not ideal 
either as if someone is enumerating the threads anyway, and likely 
performing some operation on at least some of them, the scheduler could be 
locked for some time.

Perhaps instead the function could instead return the ID of the next 
thread in the list, not the thread structure itself. Then we could use 
cyg_thread_find(). The obvious nasty with that is how badly it scales the 
further through the thread list you are. But it's safer. Obviously we 
could make it scale better, but only by introducing a fair bit more stuff 
and new data structures.

More bits below...

Nick Garnett wrote:
> +#ifdef CYGVAR_KERNEL_THREADS_LIST
> +
> +cyg_bool_t cyg_thread_get_next( cyg_handle_t *current, cyg_uint16 *id )
> +{
> +    cyg_bool_t result = true;
> +    
> +    Cyg_Scheduler::lock();
> +
> +    Cyg_Thread *thread = (Cyg_Thread *)*current;
> +
> +    if( *current == 0 )
> +    {
> +        thread = Cyg_Thread::get_list_head();
> +        *current = (cyg_handle_t)thread;
> +        *id = thread->get_unique_id();
> +    }
> +    else if( (thread->get_unique_id() == *id) &&
> +             (thread = thread->get_list_next()) != NULL )
> +    {
> +        *current = (cyg_handle_t)thread;
> +        *id = thread->get_unique_id();
> +    }
> +    else
> +    {
> +        *current = 0;
> +        *id = 0;
> +        result = false;
> +    }
> +    
> +    Cyg_Scheduler::unlock();
> +
> +    return result;
> +}
[snip]
> +cyg_bool_t cyg_thread_get_info( cyg_handle_t threadh,
> +                                cyg_uint16 id,
> +                                cyg_thread_info *info )
> +{
> +    cyg_bool_t result = true;
> +    Cyg_Thread *thread = (Cyg_Thread *)threadh;
> +    
> +    Cyg_Scheduler::lock();
> +    
> +    if( thread->get_unique_id() == id && info != NULL )
> +    {

Why pass in the id at all if it must match the thread handle?

Finally I made some doc tweaks. And incidentally in finding those I foudn 
the DNS package docbook broken and fixed that.

Jifl
-- 
eCosCentric       http://www.eCosCentric.com/       <info@eCosCentric.com>
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: doc220103.pat.txt
URL: <http://sourceware.org/pipermail/ecos-patches/attachments/20030122/64ff01b6/attachment.txt>