patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5926, Use function pointers instead of...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #5926: Use function pointers instead of table index in api_lib/api_msg

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Thu 10 May 2007 02:58:33 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  fbernon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Fri 11 May 2007 09:10:59 AM UTC, comment #10: 

Good work.

>About messages vs locking for core thread


Of course, this is not strictly realted to this patch. I'll open a new patch item once I find the time to work on it.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 May 2007 09:01:14 AM UTC, comment #9: 

Ok, it's check in. About messages vs locking for core thread, and for thread-safe problem on a same socket, I propose you to open a new patch item if it's good for you...

Frédéric Bernon <fbernon>
Group Member
Fri 11 May 2007 08:31:11 AM UTC, comment #8: 


>If it would be possible for most of "do_xxx" functions, it's not for "do_connect" which is asynchrone, and where the "end of process" (the "post") is done in err_tcp xor do_connected...


Couldn't we distinct between synchrone and asynchrone do_*() functions? In the back of my mind, I always think about locking the core instead of passing messages to a core thread. Generalizing this would make it easier to switch between the two methods of accessing core functions.

But I agree that making an exception only for do_connect() would be a bad solution.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 May 2007 08:08:50 AM UTC, comment #7: 


>But couldn't we move the sys_mbox_post() there, too?


If it would be possible for most of "do_xxx" functions, it's not for "do_connect" which is asynchrone, and where the "end of process" (the "post") is done in err_tcp xor do_connected... Perhaps "connect" should be a real exception, with a specific process (I don't think, but we can talk about that)?

>More, I think we could replace the conn->mbox by a conn->sem if we only post empty messages..


Agree (I think that most of "mbox" implementation already use "sem" inside).

Frédéric Bernon <fbernon>
Group Member
Fri 11 May 2007 07:53:38 AM UTC, comment #6: 


>But If others really prefer the "define" solution, we can...


No no, it's OK. Maybe it's really better because that way we only would have to change tcpip.c.
But couldn't we move the sys_mbox_post() there, too? Because we are waiting for the mbox in tcpip_apimsg(), I think we should post in the same file. This would also simplify the code in api_msg.c since I think there are some cases (maybe only if the programmer violates the api) where the mbox isn't posted and the calling thread would wait forever.

More, I think we could replace the conn->mbox by a conn->sem if we only post empty messages...

>But if you see something I miss, tell me..


I just wanted to know what you are talking about, seems like I missed that discussion. But no, I don't have anyting to add :-)
I think you can check it in like that (we can move the posting to the mbox later, it's not anything you change with your patch)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 May 2007 07:42:17 AM UTC, comment #5: 


>why do you mark the do_*() functions as extern? We don't have that anywhere in the stack (except for lwip_strerr() and some ppp functions). Wouldn't extern mean the functions are in another library?


It's just a thing I always do, most of time because I often mix C++ code with C code, and "extern" works for both. But yes, it's not necessary (I will remove them)

>I would favour defining api_msg_post() to tcpip_apimsg() instead of tcpip_apimsg(). That way we're still open to let it do something else and code size stays the same.


I don't like this idea, just because it's add a "function call" for nothing. One of my first idea was to use a "define" like "#define api_msg_post tcpip_apimsg", but finally, I think it's "gadget" and I prefer to show what is really call. More, because the api_msg_input is also remove, it's more "symetric" in this case. Last, If we want to decide to change the communication between api_lib and api_msg, I think it's better to directly do it in tcpip_apimsg (by example, to replace the "sys_mbox_fetch(apimsg->msg.conn->mbox, NULL);" by something else to do asynchrone calls). But If others really prefer the "define" solution, we can...

>What is it with this variable 'msg_copy'? Why was it used and why is it obsolete, now?


When we have replace in tcpip_apimsg the memp_alloc by a local variable "struct tcpip_msg msg;", someone suggest to add it (I don't remember, but the idea was perhaps due to err_tcp problem). I don't think it's necessary, because the "sys_mbox_post(msg->conn->mbox, NULL);" in api_msg is always the last instruction in a "do_xxx" function. Exception is do_connect (which wait a err_tcp or a do_connected calls). But if you see something I miss, tell me...





Frédéric Bernon <fbernon>
Group Member
Fri 11 May 2007 07:15:24 AM UTC, comment #4: 

I think replacing the 'type' field with a 'function' field is rather straightforward and that part of the patch looks good to me.

I just have three little remarks on the rest:
- Why do you mark the do_*() functions as extern? We don't have that anywhere in the stack (except for lwip_strerr() and some ppp functions). Wouldn't extern mean the functions are in another library?
- I would favour defining api_msg_post() to tcpip_apimsg() instead of tcpip_apimsg(). That way we're still open to let it do something else and code size stays the same.
- What is it with this variable 'msg_copy'? Why was it used and why is it obsolete, now?

And I agree on comment #2!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 10 May 2007 07:10:01 PM UTC, comment #3: 

If no objections, I will check in tomorrow + comment #2...

Frédéric Bernon <fbernon>
Group Member
Thu 10 May 2007 03:50:04 PM UTC, comment #2: 

I we don't need TCP, but we need the sockets API, I propose to patch lwip_sendto like this :

/*...*/
  if (sock->conn->type==NETCONN_TCP)
#if LWIP_TCP
    return lwip_send( s, data, size, flags);
#else
    sock_set_errno(sock, err_to_errno(ERR_ARG));
    return -1;   
#endif /* LWIP_TCP */
/*...*/

I will avoid to link lwip_send, netconn_write, do_write...

Same thing for lwip_send (if only TCP is need):

/*...*/
  if (sock->conn->type!=NETCONN_TCP)    
#if (LWIP_UDP || LWIP_RAW)
    return lwip_sendto( s, data, size, flags, NULL, 0);
#else
    sock_set_errno(sock, err_to_errno(ERR_ARG));
    return -1;   
#endif 
/*...*/



Frédéric Bernon <fbernon>
Group Member
Thu 10 May 2007 03:18:32 PM UTC, comment #1: 

Ok, patch file is attached. Note I have also replace api_msg_post calls by calls to tcpip_apimsg (in current CVS, the first call directly the second one). I know that tcpip_apimsg could be "one next day" replaced by tcpip_callback, but, in a first time, I think it's simplify the code (because api_msg_post is really obsolete).

I directly call the api_msg function in tcpip.c. Note I have remove the use of a "msg_copy" variable, because now, the problem with err_tcp is fixed (tell me if no objection on this point).

It's currently running and seems to work...




(file #12746)

Frédéric Bernon <fbernon>
Group Member
Thu 10 May 2007 02:58:33 PM UTC, original submission:  

Idea from "http://savannah.nongnu.org/patch/?5914" : we propose to include a function pointer instead of a table index in the message to reduce footprint. I prepare a patch file for that...

Frédéric Bernon <fbernon>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #12746:  api_msg.patch added by fbernon (12KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-11 fbernon StatusWorks For Me Done
        Open/ClosedOpen Closed
    2007-05-10 fbernon Attached File- Added api_msg.patch, #12746
        StatusNone Works For Me

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code