buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19347, netif_add & tcpip_init problem

 
 

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

bug #19347: netif_add & tcpip_init problem

Submitted by:  Frédéric Bernon <fbernon>
Submitted on:  Mon 19 Mar 2007 05:53:16 PM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Works For Me
Privacy:  Public Assigned to:  Frédéric Bernon <fbernon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

( Jump to the original submission)

Tue 22 May 2007 07:05:29 AM UTC, comment #44: 

Ok, I close it, because the DHCP timers startup can be another task...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 21 May 2007 04:54:33 PM UTC, comment #43: 

So you want it included? By now, I'm on a point where I think it can be left out (let the DHCP timer always run) since it really doesn't slow down the system that much (the time span for the DHCP timers is rather long). On the other hand, we have such a mechanism for the TCP timer already, so...
I'll include it when I find the time.

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 21 May 2007 04:46:21 PM UTC, comment #42: 

Just about your DHCP patch, see comment #35, and comment #31.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Sun 13 May 2007 11:09:36 AM UTC, comment #41: 

I think we can close this now. It "works for me", too! :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 03 May 2007 06:57:31 PM UTC, comment #40: 

Ooops, I'm sorry. But that was over a month ago ;-)

OK, forget it, then.
Sorry again.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 03 May 2007 06:44:21 PM UTC, comment #39: 

>Just a thought: can't we implement tcpip_netifapi() somehow without allocating a semaphore for each call? For example by having one semaphore per thread?


Isn't it what about we talk in comment #20 & comment #21 ?

Because these calls are not often invoked, I don't think it is necessary...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 03 May 2007 06:33:04 PM UTC, comment #38: 

Just a thought: can't we implement tcpip_netifapi() somehow without allocating a semaphore for each call? For example by having one semaphore per thread?

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 23 Apr 2007 02:13:26 PM UTC, comment #37: 

Frédéric,

> I'm agree. I will return to my office Thursday, and I hope to continue and close this item...


OK, so you can add/modify/check-in the dhcp patch if you want to, I don't specifically care about this right now, since we're not using dhcp for the moment...

About tcpip_callback(): By now, I realized what that function is meant for ;-)
I think using tcpip_callback() and a corresponding callback function would be a better way to solve this because that does not modify tcpip.h/.c and the overhead of calling a function-pointer is not that big because the netifapi-functions are not called that often... It would make the tcpip_thread smaller... What do you think?

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 22 Apr 2007 09:43:22 PM UTC, comment #36: 

>Any comments on patch file #12404 (automatically starting/stopping the dhcp timers on dhcp_start/stop())?


yes, in comment #31, it seems you don't have see it...

>I think this would be a good patch (combined with a define-switch included in opt.h and a good documentation in a new file called multithreading.txt


I'm agree. I will return to my office Thursday, and I hope to continue and close this item...

I'm currently in London for NXP presentation and training, and it seems that several people are interest about lwIP for Trimedia processor...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 20 Apr 2007 06:37:45 PM UTC, comment #35: 

Any comments on patch file #12404 (automatically starting/stopping the dhcp timers on dhcp_start/stop())?

On the one hand, it saves us the starting the timer ourselves and saves us from a running timer if there's no netif with dhcp. On the other hand, dhcp_start must be called from tcpip_thread(), so can't be called when initializing the stack (either before OS is started or from another thread). Or you have to use netifapi_dhcp_start()/stop().

I think this would be a good patch (combined with a define-switch included in opt.h and a good documentation in a new file called multithreading.txt :)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 11 Apr 2007 10:17:04 PM UTC, comment #34: 

Hi Simon,

Sorry, but I'm not at the office. Yes, a more complete solution is good, even if it will increase a little bit the footprint (for users using netifapi, of course). So, yes, continue, extend it...

About comment #31 ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 11 Apr 2007 01:37:02 PM UTC, comment #33: 

Frédéric,
I've had a quick look over the changes and it seems good to me. And you were right, a new file is the better solution ;-)

One question only: wouldn't you need some more functions to be complete? I especially think about dhcp_release() which seems to be  mandatory to call before dhcp_stop() to tell the dhcp server you are done with your address. And besides that, dhcp_inform(), netif_set_addr() (and so on) netif_set_up/down() could also be needed to be complete.

I'm already happy with the current solution, but would love to have it complete before releasing a version with this. What do you think?

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 06 Apr 2007 10:13:28 AM UTC, comment #32: 

Simon, it's check in. Send my your comments, but I won't be to my office during some days, so, I will check from home if there is any answers... About dhcp part, see comment #31...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 05 Apr 2007 12:51:10 PM UTC, comment #31: 

Ok, I join a ZIP file with two new files :

lwip/src/api/netifapi.c
lwip/src/include/lwip/netifapi.h

and the patch for tcpip.h/.c and opt.h.

Simon, any other comments about that (I just start test, but for the general design)?

About your dhcp patch, just one thing: why in start/stop timers do you use this protection :

+  SYS_ARCH_DECL_PROTECT(old_level);
+  SYS_ARCH_PROTECT(old_level);
...
+  SYS_ARCH_UNPROTECT(old_level);

You are in tcpip_thread context, and you just initialize sys_timeouts? What is the risk?

Last, a detail, it will be better to my point of view to define
DHCP_COARSE_TIMER_MSSECS to avoid to do "*1000" each time (even if most of compilers will directly optimize that...

I think we are near of the end for this problem...

(file #12404)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 04 Apr 2007 07:21:26 PM UTC, comment #30: 

Ok, I will study and give you my comments. I hope to get time to provide you a "final" patch for netifapi.c before this week end...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 04 Apr 2007 07:17:25 PM UTC, comment #29: 

Found some time for automatic dhcp timers, it's not that big anyway. May not be bug-free, only to show what I mean.

(file #12402)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 03 Apr 2007 02:35:49 PM UTC, comment #28: 

Yes it will. Ok, I continue patch like this...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 03 Apr 2007 02:33:20 PM UTC, comment #27: 

I think that's ok, yes. Since the struct will be allocated on the stack (it will, won't it?), storing some pointers is not that hard keeping in mind those functions aren't called often.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 03 Apr 2007 02:30:33 PM UTC, comment #26: 

>I don't really understand why you need to change netif_add(). Can't we simply map a call like netifapi_netif_add() to netif_add() via messages?


Yes, we can, but we just need to put in netifapi_msg all fields to do that. It will give something like :

struct netifapi_msg {
  struct netif *netif;
  err_t err;
  sys_sem_t sem;
  enum netifapi_msg_type type;
  union {
    struct {
      struct ip_addr *ipaddr;
      struct ip_addr *netmask;
      struct ip_addr *gw;
      void *state;
      err_t (* init)(struct netif *netif);
      err_t (* input)(struct pbuf p, struct netif netif);
    } add;
  } msg;
};

Is it ok for you?

>You want to implement that also? I would, but I don't have that much time at the moment...


This is not the main problem, but something to do to improve. We can do it later...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 03 Apr 2007 01:43:13 PM UTC, comment #25: 

Hi Frédéric,

re comment #15, /2:
I don't really understand why you need to change netif_add(). Can't we simply map a call like netifapi_netif_add() to netif_add() via messages?

/3:
Yes, dhcp.c would have to be modified to start/stop timer each time dhcp is started or stopped for one netif, while implementing a queue or counter to keept track of the number of netifs with dhcp active.
You want to implement that also? I would, but I don't have that much time at the moment...

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 03 Apr 2007 01:18:07 PM UTC, comment #24: 

Ok, I prepare a new patch file. I would like to get your point of view about comment #15, points 2/ & 3/ ...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 03 Apr 2007 01:06:13 PM UTC, comment #23: 

Hm, I think I really don't care :-)

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 03 Apr 2007 12:32:03 PM UTC, comment #22: 

uhmm, so, why not all in a single netifapi.c (with a minimal part in tcpip.c) ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 03 Apr 2007 11:12:42 AM UTC, comment #21: 

Yes, it's ok for me to create a semaphore each time this function is called. And including the sem into struct netifapi_msg is much better than using the conn->mbox.

Only I fear too much creation/deletion of semaphores, but on the other hand, we also do that with each creation/deletion of a socket...

With the api_msg, you're right. But you'll split it then: one half is in tcpip.c, the other half is in the new file... and I think this is only necessary if the code is 'big' enough.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 03 Apr 2007 09:09:31 AM UTC, comment #20: 

Yes, "global" semaphore is a good idea, but in this case, we can't propose a nice "integrated" netifapi for callers :

//draft
err_t
netifapi_dhcp_stop( netif* mynetif)
  struct netifapi_msg msg;
  msg.type = NETIFAPI_MSG_DHCP_STOP;
  msg.netif = my_netif;
  msg.sem   = netifapi_sem;
  if (tcpip_netifapi(&msg)!=ERR_OK) {
  /* error handler */
  }
}

Here, netifapi_sem is not defined. It can be done inside netifapi_dhcp_stop (and I think we can, because the part of the api don't have to got a high performance: we don't start/stop shcp or add/remove netif each n milliseconds, isn't it? So, a integrate semaphore is a "nice" solution. Another one is to define global semaphores and to add a netifapi_init() to initialize them. But It can cause others multithread problem (two interfaces add in the same time by example).

Your opinion about that?

>since the code has to run inside the context of a thread in tcpip.c


Yes, but it's the same for api_msg, but in a extra file.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 02 Apr 2007 06:24:40 PM UTC, comment #19: 

The second patch (file #12348) is about how I would do it. I don't think it's too bad to separate netifapi_msg stuff from api_msg, I don't like the union stuff too much, anyway...

Using the mbox of another union in the struct is bad behaviour, of course. But on the other hand, creating a semaphore with each call gives poor performance. The optimal solution would be to let the netifapi_msg point to a semaphore the user has to supply. It's not straightforward when creating the struct, but that way, one semaphore can be enough for the whole application (or one thread, at least).

Regarding different files: since the code has to run inside the context of a thread in tcpip.c, I think it's OK to put the new code into that file, also.

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 02 Apr 2007 02:50:44 PM UTC, comment #18: 

About synchrone calling, I don't have put anything in this draft to do it. In tcpip_netifapi, you can see this line :

sys_mbox_fetch(apimsg->msg.conn->mbox, NULL);

Of course, it's necessary a add a semaphore or a mailbox in netifapi_msg to wait end of process (Semaphore is the best think to do I think).

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 02 Apr 2007 02:45:05 PM UTC, comment #17: 

About LWIP_NETIF_API (which allow to enable code only when you need this part of the API), I thought the CHANGELOG file was enough, but if not, I will comment more, to explain the thread-safe problem with these functions if you want or have to call them at any time...

About a sample, yes, you're right. By example, to stop DHCP client on a netif, you can write :

...
struct netifapi_msg msg;
msg.type  = NETIFAPI_MSG_DHCP_STOP;
msg.netif = my_netif;
if (tcpip_netifapi(&msg)!=ERR_OK) {
  /* error handler */
}
...

>Out of interest, why was it necessary to define a new message type rather than just using/extending the existing API message?


I think you talk about the netconn api? I thought it was better to avoid to merge "connection api" and "network interface api". "connection api" is something generic, but "network interface api" is "linked" with the port or the target. More, it will add more fields in api_msg struct (if we have to include all netif_add parameters in the struct by example). In 1/ in comment #15, I even propose to add extra files to separate this part of the code.

Thank you to comment that...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 02 Apr 2007 02:17:18 PM UTC, comment #16: 

Some code comments to explain what the reason for the LWIP_NETIF_API is for, and how to use the API would be great, but otherwise I have no problem with this.

Out of interest, why was it necessary to define a new message type rather than just using/extending the existing API message?

Kieran Mansley <kieranm>
Project Member
Fri 30 Mar 2007 04:07:27 PM UTC, comment #15: 

I propose here a first DRAFT for a thread-safe netif api. Draft, because as is, it can be used, but it is to have a first release to talk.

This idea is to use a new define, LWIP_NETIF_API, which add types and functions in tcpip.h/.c. Four new messages are add to :

- add/remove netif
- start/stop dhcp

Several points have to be decide for this patch :

1/ Where place this code? My first idea was to add new files like netif_msg.h/.c (or netifapi_msg) but in a first time, tcpip was better. But tell me...

2/ net_add is "special", before this function do two tasks: initialize netif struct, and add it to the netif's list. I think it's not the best thing to do, but I don't want to break existing  api (even if netif_add has already change since first version). But, we can decide to change (split netif_add in two functions)  or add new functions in netif.c, or decide that init have to be done in the new function (see the code, you will understand I think). In this last case, we just have to add some fields in the new netifapi_msg struct. What is your opinion.

3/ About DHCP, we already have open patch #5798 "New DHCP messages for tcpip_thread (thread-safe problem)" https://savannah.nongnu.org/patch/?5798. Simon's idea was to avoid to start timers if no interface got dhcp active. This first patch dosen't do it, but I think the "good" place to do that would be directly in dhcp.c/.h (so need to move timers in), or, to add a dhcp_is_active to know if a dhcp client is enable.

Note I have change order in tcpip.h to have types before functions.

The new tcpip_netifapi function is synchrone.

The is a field "err" to get function result in application side.

Except any mistake, if LWIP_NETIF_API==0, nothing should be changed.

Please, comment that patch. Simon, I hope your remarks for that...

(file #12348)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Thu 29 Mar 2007 12:45:53 PM UTC, comment #14: 

> Not sure to understand. Where is it say that interface have to check if is up (with netif_is_up is suppose) to process packets? I never read that. More, if it's right, how can DHCP works? Because it is after server responses it "up" this interface. Can you give me more details about that please?


Sorry for the confusion, I did not read that anywhere, I just thought it was unnecessary processor load if a netif posted to its input function if it is down anyway. But I suppose you're right on DHCP...

I don't like the idea of adding a netif in tcpip_init_done(), though: I block the heap functions after OS is running and try to do all allocation before that. When I add the netif when tcpip_init_done() is called, ethernetif_init() is called when OS is running... (so no heap action allowed)

I would prefer the mbox!=SYS_MBOX_NULL check!

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 29 Mar 2007 12:35:57 PM UTC, comment #13: 

> I think that's not necessary: adding can be done before, only netif_set_up() has to be called in tcpip_init_done(). And the interface has to check if it is up when receiving (it should do this anyway...).


Not sure to understand. Where is it say that interface have to check if is up (with netif_is_up is suppose) to process packets? I never read that. More, if it's right, how can DHCP works? Because it is after server responses it "up" this interface. Can you give me more details about that please?

>I agree that this bug can be closed as illegal as long as we agree on waiting to enable netifs until tcpip_init_done() is called.


I prefer to close it later, after the patch for dynamic interfaces (if it's ok for you).

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 28 Mar 2007 02:07:49 PM UTC, comment #12: 

> Nevertheless, I think it's ok to just insist that netif_add should only be done after tcpip_init_done (from the callback directly or by some other means). By just making that a fact of policy, we can get rid of these checks.


I think that's not necessary: adding can be done before, only netif_set_up() has to be called in tcpip_init_done(). And the interface has to check if it is up when receiving (it should do this anyway...).

Regarding performance, you gain nothing (checking if netif is up compared to mbox!=SYS_MBOX_NULL). An besides, one null-pointer check compared to the whole mbox-processing is not that much...

Going back to the original post:
Half of this whole discussion would better fit into patch #5798, I think.
I agree that this bug can be closed as illegal as long as we agree on waiting to enable netifs until tcpip_init_done() is called.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 27 Mar 2007 10:53:41 PM UTC, comment #11: 

Ok, that's all fair enough, and I'll say that I only had 4 hours sleep last night, and that's my excuse ;-).

Nevertheless, I think it's ok to just insist that netif_add should only be done after tcpip_init_done (from the callback directly or by some other means). By just making that a fact of policy, we can get rid of these checks.

I think you using messages for this so the tcpip thread does it, but controlled by a configuration option, is an excellent idea. That completely resolves my concern - adding more code to support behaviour that most people don't need. Thanks for working on it.

And I wasn't trying to suggest that you didn't have a real job :-). From your comment, I realise it may have read that way. What I really meant was a job where one can't justify playing around with lwIP all day during work hours. But anyway, we're furiously agreeing now.

Jonathan Larmour <jifl>
Project Member
Tue 27 Mar 2007 06:02:40 PM UTC, comment #10: 

>For the avoidance of doubt, I have been doing my netif_add's in
>the tcpip_init_done callback.


Yes, I'm agree, but it's only possible for netifs available at boot time...

>rather than having to add extra tests and code in commonly used
>code paths, such as tcpip_ethinput/callback, etc. (as Frederic
>modified in that patch earlier in comment #2 to address this
>problem), it is better to change less frequently used code.


Ok, but I didn't think that check the mailbox consumne lot of cycles (but if you think that, I can add a macro, or remove them if I implement new messages).

>I have a question though - where does the tcpip thread access
>the netif list? I didn't think it did.


Simple question, simple answer : EVERYWHERE !!! (joke). Just do a grep on "netif_list" :
ip_input (ipv4&ipv6)
ip_route (ipv4&ipv6)
dhcp_coarse_tmr (dhcp)
dhcp_fine_tmr (dhcp)
ip_addrentry_get_value (snmp)
ip_rteentry_get_value (snmp)
snmp_ifindextonetif (snmp)
snmp_netiftoifindex (snmp)
igmp_init (igmp)

I don't think it's a good idea to "add extra tests and code in commonly used code paths", isn't it :) ? So, add semaphores in all these processings is not a good idea, that's why I have propose to add new messages, because the "switch (msg->type)" won't be slower is you set these messages at the end of the switch (with most of compilers). More, if this extra processing isn't necessary we can easily disable it with a configuration option.

>About the times: I'm sure most developers here have real jobs


Why, you don't have a real job? ;) Integrate lwIP in our products is a part of my job, and I suppose that lot of users have the same aim. The problem is there is no lot of "real" active developers on lwIP (23 in the list, but only 5-6 on the forum most of time), and if we have to wait that all give a response, sometimes, we have to wait a long, long time... Sometimes, some threads don't have any comments. Kieran have accept to be project leader, even if no longer use lwIP himself, and he take time to response to lot of requests (and it's wonderfull, thank you again for that), but not all, and it's normal. In this case, I suppose we can decide to commit without wait more (I also suppose that's why he give us write access to CVS). There is no "offical" rules about check in to CVS, so 24h or Kieran's agreement are rules I will follow now.

About informations, I think that projects where team talk and know how everyone works, help to have good results. I already have "private" emails with some developers to understand their jobs, and how they use lwIP. So, that's why I have given some informations about my job. But, last, it's just a point of view...

Tomorrow, I will commit some patchs, and submit new ones. I hope you can tell me what you think about them...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 27 Mar 2007 04:42:23 PM UTC, comment #9: 

Okay, I see what you are trying to solve - not (necessarily) the initial netif_adds, but later dynamic ones.

For the avoidance of doubt, I have been doing my netif_add's in the tcpip_init_done callback.

I would say that rather than having to add extra tests and code in commonly used code paths, such as tcpip_ethinput/callback, etc. (as Frederic modified in that patch earlier in comment #2 to address this problem), it is better to change less frequently used code. So for example, if changes to the netif list needs protection from access by multiple threads, then it should be protected by a semaphore (or use SYS_ARCH_PROTECT).

I have a question though - where does the tcpip thread access the netif list? I didn't think it did.

About the times: I'm sure most developers here have real jobs which means we can't monitor the lists every second of the day ;-), and we have varying timezones and working hours (I can usually play with lwIP more in the evenings than the daytime for example). So we can't be guaranteed to respond quickly at any particular point. That's all I was meaning really. If something could be controversial and needs further opinions, then it's better to wait a bit longer, that's all. And since you've already said your happy with that, all is well :-).

Jonathan Larmour <jifl>
Project Member
Fri 23 Mar 2007 08:56:36 AM UTC, comment #8: 

I'm agree with Simon (and in fact, it was in another thread about dhcp_stop/start we have talk about that). Changes will add some source code, but no footprint increase if yor don't enable the option (which will be disable per default). Even, performance don't change...

Perhaps the title is not good "netif lists change & tcpip_init" would be better?

I give you some samples, and I'm interrest about the way you solve them (it's good if there is another solution, I don't think I got all the good solutions :) ):

- your network interface is initialized before tcpip_init, and it receive a packet, what is done to avoid to use an uninitialized tcpip's mbox?

- you have dynamic network interfaces, you want to add a netif when you plug a pcmcia card (wifi, umts,...), and to remove it when you unplug it. How to avoid to crash if you remove it from netif'list (tcpip_thread access to this list, and netif_remove can have a concurrent access with it)? Perhaps, a alternative solution (but I don't like it) will be to add all interfaces - even if there are not plugged - and to handle inside the driver the plug/unplug operations?

- your network interface is initialized after tcpip_init, even it this case seems to not cause a problem with list operations, what will happen if two thread try to add in the same time a netif (power up time by example) ? Look these two lines from netif_add:

 

  netif->next = netif_list;
  netif_list = netif;

I have the risk than one of the interface was not insert in the list (say the netif1 is interface for add by thread1, netif2 by thread2):

  

   netif1->next = netif_list;
   netif2->next = netif_list;
   netif_list = netif2;
   netif_list = netif1;
   !!! netif2 is not in the list !!!!

Last, about 24h delay, you are right. But Monday & Thursday, except Kieran, there wasn't lot of active developpers "inline" (even you wasn't "inline" seems some weeks...). I know that everyone have to take time (vacation & others), but even 24h seems short. But I will respect that, and I wait more next time...

Perhaps it would be useful to know how (and when?) each one work and use lwIP?

So, I'm working in the south of the France (GMT+1+daylight), and I'm using Ethernet interfaces (and next, Wifi), using lwIP sockets layer, on a DSP platform (NXP), using pSOS/OSAL. I don't use internal lwIP features (sys.h interface) in my application level (no timeouts, and I directly use my OS Semaphore, Queue, etc...).  I prefer to use sockets layer because I want to be able to port my applications & protocols in another stack or environment is need. My products have to be used in any network environment (private network, internet, receiving/sending lot of packets, ~4Mbps ...). Network settings have to be done in runtime (dhcp/static, etc...).

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Fri 23 Mar 2007 07:59:34 AM UTC, comment #7: 

Jonathan,

Unfortunately, I didn't have the time to follow the changes. I only know the idea behind this: If you want to change options of an interface (e.g. change the IP address), you probably do this asynchronously, say from another application thread. But if you do that, you have concurrent access to the netif-list or -struct because you access this from tcpip-thread as well as from your application thread.

Changes made here should not change the lwip core (only the api part) and raw-only users should not see a change.

This is a general problem about multithreading and lwIP, as you stated in your last comment to patch #5453...
Since the stack has no protection in it, any operation of application threads on the stack's structs can only work through messages.

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 23 Mar 2007 07:19:42 AM UTC, comment #6: 

Hold on, step back here. What problem are you trying to solve?

What you are describing sounds like a problem in your port, not in lwIP. I, and I'm sure many others, have no problems with netif_add and tcpip_init in a multi-thread environment.

I'm concerned that you are getting ahead of yourself adding potentially unnecessary code to lwIP.

I'm also concerned that you wanted to open this for other comments, but only waited 24 hours before checking things in!

Anyway, since I personally have no problems with netif_add running after tcpip_init, I would be interested in knowing what the problem you are trying to fix really is.

Jonathan Larmour <jifl>
Project Member
Wed 21 Mar 2007 07:50:39 PM UTC, comment #5: 

Hello Simon,

You're right !!! I've done it in my driver, but I have forget to update ethernetif skeleton and to comment this point ...

I will do...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 07:44:08 PM UTC, comment #4: 

If you don't take the packet (in tcpip.c) in tcp_input() and tcpip_ethinput(), shouldn't you free the pbuf? If not, ethernetif would have to evaluate the return value of the netif->input() function and free the packet on error.

Simon Goldschmidt <goldsimon>
Project Administrator
Tue 20 Mar 2007 06:03:59 PM UTC, comment #3: 

Patch submit. tcpip's mbox is checked. But Adding hotplug interfaces is always a problem.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 20 Mar 2007 03:24:31 PM UTC, comment #2: 

I propose you this patch to verify before any post the mbox variable. The file is join. Is no objection, I will commit it...

(file #12219)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 20 Mar 2007 08:20:31 AM UTC, comment #1: 

A first thing to do will be to initialize in tcpip.c the mbox like this :

static sys_mbox_t mbox = SYS_MBOX_NULL;

and in each tcpip_xxx, check mbox like this :

err_t
tcpip_input(struct pbuf p, struct netif inp)
{
  struct tcpip_msg *msg;

  if (mbox != SYS_MBOX_NULL) {
    msg = memp_malloc(MEMP_TCPIP_MSG);
    if (msg == NULL) {
      pbuf_free(p);
      return ERR_MEM;
    }

    msg->type = TCPIP_MSG_INPUT;
    msg->msg.inp.p = p;
    msg->msg.inp.netif = inp;
    sys_mbox_post(mbox, msg);
  }

  return ERR_OK;
}

Comments ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 19 Mar 2007 05:53:16 PM UTC, original submission:  

I've already talk about that with Simon, but I would like to get some other comments about initialization time.

netif_add and tcpip_init have a problem in a multithread environment:

If the network interface is added before tcpip_init is called, then, if the driver have packets to send to tcpip_thread, it can call the tcpip_thread's mbox before it was initialized and can cause a crash (or perhaps it's something to check in sys_arch? It's something I would like to add in documentation).

If the driver is added after tcpip_init, then, there is no possibility to do it in the tcpip_thread context (which use netif'list in most processing). Exception, you can do it in the tcpip_init_done callback, but only one time (if you have any "dynamic" network interface, it can be a problem).

I think the "good" thing to do is to add new tcpip_msg_types to be able to add/remove an interface at any time after tcpip_init (with configuration option is not needed).

Last, when I look in some ports, it seems that netif_init() is not called in "simnode" & "simrouter". In "unixlib.c", the netif_init() is done AFTER tcpip_init (it should be call BEFORE).

About this initialisation problem, "simhost" & "unixlib" add network interfaces AFTER tcpip_init, and "simnode" & "simrouter" BEFORE.

So, can you provide some comments about the way you actually do your initialisation, and what you think about this "problem"?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.

 

Attached Files
file #12404:  netifapi.zip added by fbernon (4KiB - application/zip)
file #12402:  dhcp.c.patch added by goldsimon (3KiB - text/x-patch)
file #12348:  netifapi.patch added by fbernon (7KiB - text/x-patch)
file #12219:  tcpip.c.patch added by fbernon (4KiB - 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 jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-22 fbernon Open/ClosedOpen => Closed
    2007-04-06 fbernon StatusIn Progress => Works For Me
    2007-04-05 fbernon Attached File- => Added netifapi.zip, #12404
    2007-04-04 goldsimon Attached File- => Added dhcp.c.patch, #12402
    2007-03-30 fbernon StatusNone => In Progress
    2007-03-30 fbernon Attached File- => Added netifapi.patch, #12348
    2007-03-20 fbernon Attached File- => Added tcpip.c.patch, #12219

    Back to the top


    Powered by Savane 3.5