This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: Kernel KAPI and instrumentation mods

From: Jonathan Larmour <jifl at eCosCentric dot com>
To: Nick Garnett <nickg at ecoscentric dot com>
Cc: ecos-patches at sources dot redhat dot com
Date: Wed, 22 Jan 2003 06:38:37 +0000
Subject: Re: Kernel KAPI and instrumentation mods
References: <m3fzt3hyqw.fsf@ecoscentric.com>

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

Index: kernel/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/ChangeLog,v
retrieving revision 1.87
diff -u -5 -p -r1.87 ChangeLog
--- kernel/current/ChangeLog	13 Jan 2003 05:50:23 -0000	1.87
+++ kernel/current/ChangeLog	22 Jan 2003 06:38:22 -0000
@@ -1,5 +1,9 @@
+2003-01-22  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* doc/kernel.sgml: Document cyg_thread_info type.
+
 2003-01-13  Dmitriy Korovkin  <dkorovkin@rambler.ru>
 2003-01-13  Jonathan Larmour  <jifl@eCosCentric.com>
 
 	* include/mqueue.hxx: Allow get/put to return time out.
 	* include mqueue.inl: Ditto.
Index: kernel/current/doc/kernel.sgml
===================================================================
RCS file: /cvs/ecos/ecos/packages/kernel/current/doc/kernel.sgml,v
retrieving revision 1.5
diff -u -5 -p -r1.5 kernel.sgml
--- kernel/current/doc/kernel.sgml	12 Dec 2002 18:31:38 -0000	1.5
+++ kernel/current/doc/kernel.sgml	22 Jan 2003 06:38:25 -0000
@@ -1362,11 +1362,11 @@ run the specified thread has not yet bee
 point in the function call graph. Never the less the value returned
 can give some useful indication of the thread's stack requirements.
       </para>
       <para>
 <function>cyg_thread_get_next</function> is used to enumerate all the
-current threads in the system. It should be called intially with the
+current threads in the system. It should be called initially with the
 locations pointed to by <parameter>thread</parameter> and
 <parameter>id</parameter> set to zero. On return these will be set to
 the handle and ID of the first thread. On subsequent calls, these
 parameters should be left set to the values returned by the previous
 call.  The handle and ID of the next thread in the system will be
@@ -1379,10 +1379,30 @@ indicates the end of the list.
 thread described by the <parameter>thread</parameter> and
 <parameter>id</parameter> arguments. The information returned includes
 the thread's handle and id, its state and name, priorities and stack
 parameters. If the thread does not exist the function returns
 <literal>false</literal>.
+    </para>
+    <para>
+The <type>cyg_thread_info</type> structure is defined as follows by
+&lt;<filename class=headerfile>cyg/kernel/kapi.h</filename>&gt;, but may
+be extended in future with additional members, and so its size should
+not be relied upon:
+<programlisting>
+typedef struct
+{
+    <type>cyg_handle_t</type>        <structfield>handle</structfield>;
+    <type>cyg_uint16</type>          <structfield>id</structfield>;
+    <type>cyg_uint32</type>          <structfield>state</structfield>;
+    <type>char</type>                <structfield>*name</structfield>;
+    <type>cyg_priority_t</type>      <structfield>set_pri</structfield>;
+    <type>cyg_priority_t</type>      <structfield>cur_pri</structfield>;
+    <type>cyg_addrword_t</type>      <structfield>stack_base</structfield>;
+    <type>cyg_uint32</type>          <structfield>stack_size</structfield>;
+    <type>cyg_uint32</type>          <structfield>stack_used</structfield>;
+} cyg_thread_info;
+</programlisting>
     </para>
     <para>
 <function>cyg_thread_find</function> returns a handle for the thread
 whose ID is <parameter>id</parameter>. If no such thread exists, a
 zero handle is returned.
Index: net/ns/dns/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/ns/dns/current/ChangeLog,v
retrieving revision 1.10
diff -u -5 -p -r1.10 ChangeLog
--- net/ns/dns/current/ChangeLog	18 Jan 2003 04:22:59 -0000	1.10
+++ net/ns/dns/current/ChangeLog	22 Jan 2003 06:38:25 -0000
@@ -1,5 +1,9 @@
+2003-01-22  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* doc/dns.sgml: Use correct (and working!) docbook.
+
 2003-01-18  Jonathan Larmour  <jifl@eCosCentric.com>
 
 	* include/dns_impl.inl (setdomainname): define with const name
 	argument.
 	* include/dns.h: Ditto.
Index: net/ns/dns/current/doc/dns.sgml
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/ns/dns/current/doc/dns.sgml,v
retrieving revision 1.4
diff -u -5 -p -r1.4 dns.sgml
--- net/ns/dns/current/doc/dns.sgml	18 Jan 2003 04:23:00 -0000	1.4
+++ net/ns/dns/current/doc/dns.sgml	22 Jan 2003 06:38:25 -0000
@@ -88,13 +88,24 @@ or user code my override this address by
 <literal>cyg_dns_res_init</literal> again. </PARA>
 
 <PARA>The DNS client understands the concepts of the target being
 in a domain. By default no domain will be used. Host name lookups
 should be for fully qualified names. The domain name can be set
-and retrieved using the functions:<PARA> 
-<PROGRAMLISTING>setdomainname(const char *name, size_t len);
-getdomainname(char *name, size_t len);</PROGRAMLISTING>
+and retrieved using the functions:
+<funcsynopsis>
+  <funcprototype>
+    <funcdef>int <function>getdomainname</function></funcdef>
+    <paramdef>char *<parameter>name</parameter></paramdef>
+    <paramdef>size_t <parameter>len</parameter></paramdef>
+  </funcprototype>
+  <funcprototype>
+    <funcdef>int <function>setdomainname</function></funcdef>
+    <paramdef>const char *<parameter>name</parameter></paramdef>
+    <paramdef>size_t <parameter>len</parameter></paramdef>
+  </funcprototype>
+</funcsynopsis>
+</para>
 
 <PARA>Alternatively, a hard coded domain name can be set using CDL.
 The boolean <literal>CYGPKG_NS_DNS_DOMAINNAME</literal> enables this
 and the domain name is taken from
 <literal>CYGPKG_NS_DNS_DOMAINNAME_NAME</literal>.</PARA>

Follow-Ups:
- Re: Kernel KAPI and instrumentation mods
  - From: Nick Garnett

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]