buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19345, tcpip_apimsg+api_msg_post problem

 
 

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

bug #19345: tcpip_apimsg+api_msg_post problem

Submitter:  Frédéric Bernon <fbernon>
Submitted:  Mon 19 Mar 2007 04:53:45 PM UTC
   
 
Category:  None Severity:  4 - Important
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  fbernon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Wed 21 Mar 2007 05:09:34 PM UTC, comment #8: 

Commited  with:

void
api_msg_input(struct api_msg *msg)
{ struct api_msg_msg msg_copy;
msg_copy=msg->msg;
decode[msg->type](&msg_copy);
}

Frédéric Bernon <fbernon>
Group Member
Wed 21 Mar 2007 05:09:06 PM UTC, comment #7: 


>If you use the local variable to send tcpip_msg, it would still leave
>you with potential problems. because you didn't create an copy for this
>variable, the local variable will be unavliable when the program run to
>other area.


It "could be" true, but don't forget the tcpip_apimsg's call is block (inside the "struct tcpip_msg msg;" scope) by "sys_mbox_fetch(apimsg->msg.conn->mbox, NULL)" until tcpip_thread post it a "acknowledge" (and in all api_msg "do_xxx"; the "sys_mbox_post(msg->conn->mbox, NULL);" is "always" at the end). So, the variable can't be used outside the scope of tcpip_apisg. The "generic" process is that (simplified) :

App Thread
tcpip_apimsg()
{ struct tcpip_msg msg
  post to tcpip_thread
  sys_mbox_fetch(start wait...

                              tcpip_thread()
                              { sys_mbox_fetch()
                                  api_msg_input()
                                    do_xxxx()
                                     { // message process
                                       sys_mbox_post(msg->conn->mbox, NULL);
                                     }
                              }
  End wait...)
}

I say "always", but there is some exceptions. This is my check list :

  do_newconn          => sys_mbox_post(msg->conn->mbox, NULL) at the end, but if (msg->conn->pcb.tcp != NULL), it is called immediatly
  do_delconn          => sys_mbox_post(msg->conn->mbox, NULL) at the end, but with a (msg->conn->mbox != SYS_MBOX_NULL) check
  do_bind             => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_connect          => multiples sys_mbox_post(msg->conn->mbox, NULL) with return just after
  do_disconnect       => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_listen           => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_accept           => !!!! no "direct" process? And YES, this can be a problem... But in fact, do_accept do nothing, can its call could be avoid, because never use (will have to be suppress)...
  do_send             => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_recv             => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_write            => sys_mbox_post(msg->conn->mbox, NULL) at the end 
  do_close            => sys_mbox_post(msg->conn->mbox, NULL) at the end
  do_join_leave_group => sys_mbox_post(msg->conn->mbox, NULL) at the end

Some extras calls:
  do_connected:       => when a tcp connection is really connected, sys_mbox_post(msg->conn->mbox, NULL) est call.
  err_tcp             => called when tcp error, and all conn's mailboxes receive a NULL message (recvmbox, mbox, acceptmbox).


Last, I you really want to be sure, you can write something like :

void
api_msg_input(struct api_msg *msg)
{ struct api_msg_msg msg_copy;
  msg_copy=msg->msg;
  decode[msg->type](&msg_copy);
}

This will stay faster than using memp...

About this part, agree to commit?


>Do you meet the condition that MEMP_TCP_SEG can not be free in some
>case.

I don't understand this question (in this context) ? But no, I don't meet this condition. Of course, I will tell you if I see something like that...

Frédéric Bernon <fbernon>
Group Member
Wed 21 Mar 2007 11:55:16 AM UTC, comment #6: 

Seems OK to me.

Kieran Mansley <kieranm>
Group Member
Tue 20 Mar 2007 07:44:50 PM UTC, comment #5: 

I join a patch to solve the problem, and to improve reliability (and code is more simple).

"api_lib.c, tcpip.c: integrate sys_mbox_fetch(conn->mbox, NULL) calls from api_lib.c to tcpip.c's tcpip_apimsg(). Now, use a local variable and not a dynamic one from memp to send tcpip_msg to tcpip_thread in a synchrone call. Free tcpip_msg from tcpip_apimsg is not done in tcpip_thread. This give a faster and more reliable communication between api_lib and tcpip."

Patch file is attached.

If no objection, I will commit it tomorrow...


(file #12223)

Frédéric Bernon <fbernon>
Group Member
Tue 20 Mar 2007 09:55:35 AM UTC, comment #4: 

Other solution (if someone read it?):

remove MEMP_TCPIP_MSG's alloc/free feature. Because only "api" used it we could change like this :

err_t
tcpip_apimsg(struct api_msg *apimsg)
{
  struct tcpip_msg msg;
  msg->type = TCPIP_MSG_API;
  msg->msg.apimsg = apimsg;
  sys_mbox_post(mbox, msg);
  sys_mbox_fetch(apimsg->msg.conn->mbox, NULL);
  return ERR_OK;
}

and change at the en of tcpip_thread this line:

memp_free(MEMP_TCPIP_MSG, msg);

by this one :

if (msg->type!=TCPIP_MSG_API)
  memp_free(MEMP_TCPIP_MSG, msg);

It will be faster, footprint will not be increased and more reliable (no "not enought memory" problem...). Even, I suggest to directy call tcpip_apimsg from api_lib.c, because it would be faster and design would be simple (or we can use a define?)...

Hoping comments ?


Frédéric Bernon <fbernon>
Group Member
Tue 20 Mar 2007 08:27:21 AM UTC, comment #3: 

api_msg_post() is only used in api_lib.c. Each call is the same sequence:

Prepare message
api_msg_post(&msg);
sys_mbox_fetch(conn->mbox, NULL);

I propose to change api_msg_post like this :

err_t
api_msg_post( struct netconn *conn, struct api_msg *msg)
{ msg->msg.conn = conn;
  if (tcpip_apimsg(msg)==ERR_OK)
   { sys_mbox_fetch(conn->mbox, NULL);
   }
  return ERR_MEM;
}

And to change each call in api_lib.c like this:

err_t
netconn_disconnect(struct netconn *conn)
{
  struct api_msg msg;
 
  if (conn == NULL) {
    return ERR_VAL;
  }

  msg.type = API_MSG_DISCONNECT;
  api_msg_post( conn, &msg);

  return conn->err;

}

Your comments?


Frédéric Bernon <fbernon>
Group Member
Mon 19 Mar 2007 08:37:04 PM UTC, comment #2: 

First step is done. Before change api_lib.c, is there any comments?


Frédéric Bernon <fbernon>
Group Member
Mon 19 Mar 2007 05:09:31 PM UTC, comment #1: 

Sounds like a good idea to me.

Kieran Mansley <kieranm>
Group Member
Mon 19 Mar 2007 04:53:45 PM UTC, original submission:  

tcpip_apimsg & api_msg_post functions don't have any return type. So, if we got any memp_malloc error, api_msg_post's caller doesn't know that the message is not sent. And in api_lib.c, we always wait an answer with a sys_mbox_fetch(conn->mbox, NULL); But tcpip_thread will never answers, because he never got the message.

This is not a new problem. I can find in forum some threads :

- "bug&patch: when an api_msg is lost tcp get stuck" (May 2005)
- "API code issues" (May 2006)

The first thing to change in api is to change return types from "void" to "err_t" for tcpip_apimsg & api_msg_post (like this):

err_t
tcpip_apimsg(struct api_msg *apimsg)
{
  struct tcpip_msg *msg;
  msg = memp_malloc(MEMP_TCPIP_MSG);
  if (msg == NULL) {
    return ERR_MEM;
  }
  msg->type = TCPIP_MSG_API;
  msg->msg.apimsg = apimsg;
  sys_mbox_post(mbox, msg);
  return ERR_OK;
}

err_t
api_msg_post(struct api_msg *msg)
{
  return tcpip_apimsg(msg);
}

The second thing is to change in api_lib.c each call like this :

  ...
  if (api_msg_post(&msg)==ERR_OK)
   { sys_mbox_fetch(conn->mbox, NULL);
     ...
   }
  else
   { ... error handler
   }
  ...

Good for you?



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 #12223:  tcpip_apimsg.patch added by fbernon (6KiB - 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 kieranm (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-03-21 fbernon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2007-03-20 fbernon Attached File- Added tcpip_apimsg.patch, #12223

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code