buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52770, mdns: move the announcements to a...

 
 

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

bug #52770: mdns: move the announcements to a timer callback

Submitter:  Douglas <ourairquality>
Submitted:  Sun 31 Dec 2017 07:33:25 AM UTC
   
 
Category:  DNS Severity:  3 - Normal
Item Group:  Change Request Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Thu 04 Jan 2018 07:04:48 AM UTC, comment #24: 

This is more or less a duplicate to bug #48953.
And since it seems noone continues here, I'll close this for now.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 03 Jan 2018 07:18:39 PM UTC, comment #23: 

You say stack usage is the main problem. Wouldn't the simplest solution for you be to configure your netif via tcpip_callback() instead of from your application thread using the netif API? This should solve your problem, or am I wrong?

Just allocate an object that carries the new config using mem_malloc and fill it in. Then do
tcpip_callback(my_netif_config_function, pointer_to_some_object_containing_new_config) from your application thread. In my_netif_config_function(), apply the new config and free the object again. Wouldn't that work for you???

Agreed, its not very elegant, but you can create a small wrapper function that hides the uglyness and it would save you any optimization in lwIP.

I don't know your application, maybe this is not possible for you?

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 03 Jan 2018 07:02:06 PM UTC, comment #22: 


> Some things are hard to respond to delicately


I know, beleive me.

I said this before, and I don't mean it rude: given your concerns about stack usage outside from tcpip_thread, I guess core locking is just not suited for your case. I don't see a point in using a timer on all platforms when the reason for that change is to keep stack usage on application threads low. There has to be a better solution for that ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 03 Jan 2018 03:02:25 PM UTC, comment #21: 


> Douglas, any response to my comment #17?


> Again, we're not talking of the same thing. What I meant was that with the current solution, you (Douglas, not "one") can implement the exact delay you want.


Telling people to not use LWIP_NETIF_EXT_STATUS_CALLBACK and schedule it all themselves. Some things are hard to respond to delicately.

> What you proposed was to let everyone have an arbitrary delay of 50ms before an announcement is sent. That might fit your needs. What if this is too long or too short for the next user? To me, this looks like a solution to your specific setup, not a generic fix.


The delay was not the key part of the change, it could be zero. Someone else suggested it would be good to have a random delay, so I added support for that. I added an option to control this too, so the timer was skipped and it was sent immediately in the callers context.

> OK, got that. What I did not get yet is where do the multiple announcements come from? Changing IPv4 address/netmask/gateway triple should only result in one announcement. If it doesn't, let's fix this. Or is it something else that's wrong for you?


The problem is not multiple announcements. It would not bother me if there were a bunch of announcements when a change was make. There were some issues with it announcing empty ip address but that was resolved. The point was that they trigger an announcement in the context of the caller, and that that was a burden on the callers stack. Look back to the original comment in the bug and that should be clear see:

> The guts of the mdns code uses a significant amount of stack for a small system. This makes it problematic calling into this code from the various paths that trigger announcements. This patch has these code paths queue an announcement that will be performed via a timer callback.





Douglas <ourairquality>
Wed 03 Jan 2018 02:34:04 PM UTC, comment #20: 

Douglas, any response to my comment #17?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 02 Jan 2018 07:34:22 PM UTC, comment #19: 

I'm OK with using a timer if we need one. But even in this case, we would start the collision test phase as soon as possible, right? Not after an arbitrary delay. I still say that 50ms delay is hiding things we should fix. That's why I still take part in this discussion.

This like this are spread accross the stack. E.g. converting to core locking, DNS queries are sent right away (from the calling thread), not from any later point. That's the whole point in core locking. Maybe we should leave the message passing thing in instead of removing it, so people having concerns about the stack usage can stay with one stack for the lwip core...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 02 Jan 2018 07:28:24 PM UTC, comment #18: 

I think the timer is a reasonable start. Before the announcement is done, we really should query our names for collisions - and this will need a timer.

Erik Ekman <yarrick>
Group Member
Tue 02 Jan 2018 07:01:27 PM UTC, comment #17: 


>> You can pass 'mdns_resp_announce' to tcpip_callback() (which
>> won't get removed!) or sys_timeout(). That will fix your
>> stack requirements.
> Yes, that was my proposed solution.


Again, we're not talking of the same thing. What I meant was that with the current solution, you (Douglas, not "one") can implement the exact delay you want.

What you proposed was to let everyone have an arbitrary delay of 50ms before an announcement is sent. That might fit your needs. What if this is too long or too short for the next user? To me, this looks like a solution to your specific setup, not a generic fix.

> I have explained the problem a few times now, giving the
> example of the ipv4 address changing and triggering an
> announcement.


OK, got that. What I did not get yet is where do the multiple announcements come from? Changing IPv4 address/netmask/gateway triple should only result in one announcement. If it doesn't, let's fix this. Or is it something else that's wrong for you?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 02 Jan 2018 12:24:02 PM UTC, comment #16: 


> You can pass 'mdns_resp_announce' to tcpip_callback() (which won't get removed!) or sys_timeout(). That will fix your stack requirements.


Yes, that was my proposed solution. What landed was:

> I just committed that adding netifs/services does not automatically announce any more.


I have explained the problem a few times now, giving the example of the ipv4 address changing and triggering an announcement. This is not addressed by the change that landed.

It could work if every caller that triggers an ipv4 address change also queues a 'mdns_resp_announce', but that would appear to defeat the purpose of having the LWIP_NETIF_EXT_STATUS_CALLBACK.

Further the change that landed is not necessary if with the solution proposed, and just creates a greater burden for the caller (to have to explicitly trigger an announcement after adding a service).

> I always think it's better to keep things in sync.


Yes, I make an effort to try to sync changes upstream for the benefit of the wider community. But there are a lot of other issues, so I don't want to get bogged down on this. The esp8266 might be an extremely limited device for lwip, for example it seems to have too little ram for pool allocation. Even an extra 1/2 KB stack burden across a few threads is a problem. These might not seem like issues for the larger lwip community so perhaps some differences are expected. So the only other suggestion would be to make it optional to schedule the announcement, so that the source differences were minimal?

Douglas <ourairquality>
Tue 02 Jan 2018 11:34:28 AM UTC, comment #15: 

I'm afraid I don't understand. What's missing for you now? You can pass 'mdns_resp_announce' to tcpip_callback() (which won't get removed!) or sys_timeout(). That will fix your stack requirements. Or is stack usage still too high?

Next, you can check the netif state to see if the netif is up already (so you have to issue an announcement) or no (the announcement will be sent once it's up). Or is this just too uncomfortable?

> The netifapi will be deprecated but retained for backwards compatibility.


That's a vague plan, not a fixed roadmap. Unless it's done, don't rely on it!

> I'll probably remove it from the esp-open-rtos fork


I always think it's better to keep things in sync. Remember we are trying to understand what you want and why and we have to ensure the stack stays usable for many more platforms than yours.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 02 Jan 2018 11:03:59 AM UTC, comment #14: 


> is your problem solved now?


No, and I believe a good faith effort has been made to explain why. Please re-read the comments if really interested in the reasons.

Bottom line is I need something that works for esp-open-rtos. The feedback has improved the patches a lot, thank you, but I might need to use a different solution. Perhaps they can converge in future.

Regarding mdnsapi_mdns_resp_announce(), it was explained elsewhere that core locking will become mandatory with threading and that the caller is expected to ensure the core lock is held for callback style apps such as mdns. The netifapi will be deprecated but retained for backwards compatibility. So there seems no point adding more entries in that style such as mdnsapi_mdns_resp_announce(). I'll probably remove it from the esp-open-rtos fork so people don't use it and get as confused as I was, rather give some examples that acquire the core lock before calling into the mdns app.

Douglas <ourairquality>
Tue 02 Jan 2018 10:04:41 AM UTC, comment #13: 

I just committed that adding netifs/services does not automatically announce any more. You can now call mdns_resp_announce() via mdnsapi_mdns_resp_announce() or using tcpip_callback() - is your problem solved now?

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 02 Jan 2018 09:46:43 AM UTC, comment #12: 


> What I meant was the administative "up" of a network - before calling "netif_set_up()", not before the link is up or an IP was assigned.


There can still be changes that affect the mdns state and would trigger an announcement. e.g. The ipv4 address being obtained via dhcp etc. It does not appear to be practical to expect the mdns state to be statically set so early.

If it was just a matter of not allowing mdns services to be added once the network is 'up', then that could have been addressed by the original suggestion of not triggering the announcement when adding a service. Unfortunately that was not the only path that triggers an announcement.

Sometimes different threads configure different parts of lwip. The netif might be created in some startup core library (for the esp-open-rtos in a SDK library), and an application layer thread might setup mdns where is installs the TXT callback. So this use case pattern would also not fit with having mdns configured so early.

Douglas <ourairquality>
Tue 02 Jan 2018 09:30:31 AM UTC, comment #11: 

What I meant was the administative "up" of a network - before calling "netif_set_up()", not before the link is up or an IP was assigned.

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 02 Jan 2018 09:20:34 AM UTC, comment #10: 


> NUM_TIMOUTS needs to be increased


Thank you, that had not occurred to me, probably as I am not using pools. Still, that seems a small burden compared with expecting all the callers to provision far more space in stack memory.

> Add all services before the netif is up - I think this can be done in most systems.


The original suggestion was to simply not send the announcements from the set up functions, and that would have had a similar effect. Then add another public function to trigger the announcement.

Unfortunately this can not solve all the problems because the netif can by up and running ipv6 before it gets an ipv4 address so there are still these paths to deal with ipv4 can still change while the network is up. Some caller has set the ipv4 address, probably another thread, and don't want it to have to provision for the stack needs of mdns.

Scheduling the announcement seems a fine solution to me, and the timer is the simple path as it is the only solution to also support NO_SYS.

Anyway we don't have to agree, I can keep that small aspect on a fork, or would you consider a patch that could work either way?

> Add functions "mdns_stop_announce()" and "mdns_resume_announce()" to temporarily stop/resume announcing while services are being added


It's not just when services are added, but also on network changes. It does not seem practical to expect every caller that might trigger an mdns announcement to guard with these, and changing state in that way looks like a thread race problem.

Douglas <ourairquality>
Tue 02 Jan 2018 08:23:46 AM UTC, comment #9: 

I don't like to add another timer for this, its not worth it (yet another thing to configure in lwIP for MDNS - NUM_TIMOUTS needs to be increased). There are two solutions:

- Add all services before the netif is up - I think this can be done in most systems

- Add functions "mdns_stop_announce()" and "mdns_resume_announce()" to temporarily stop/resume announcing while services are being added

Would one of these be feasible for you?

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 31 Dec 2017 01:40:38 PM UTC, comment #8: 


> Was the intention for the LWIP_ERROR check to always be
> performed, but perhaps only verbose in debug builds?


That's the plan. Also assertions for other things that can be done wrong regarding threading. I'm not sure a bool is enough here though.

> Will that mean the netifapi entry points are removed too?


No, backwards compatibility is still a requirement of course. Unless it get's too bad to keep it. But keeping those functions around for a while should not hurt...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 31 Dec 2017 01:37:18 PM UTC, comment #7: 


> So your starting to use core locking for a callback style application does not really fit to lwIP the way it was.


Thank you that helps. So I'll remove the mdns thread safe entry points, and make a note that the caller is expected to hold the core lock.

> Would it be too hard to write your application to call LOCK_TCPIP_CORE() before calling into mdns?


Yes, that seems workable.

Could assertions be added at some of these entry points, that require the core lock held, to check that it really is? Might this be implemented within lwip by a simple flag set by the LOCK_TCPIP_CORE() macro and cleared when unlocked?

> Core locking is already the default and message passing should be removed in one of the next releases, so you're OK with only implementing core locking code.


Will that mean the netifapi entry points are removed too? I just started to use these, but should the caller be using LOCK_TCPIP_CORE() rather than these if they are a dead end?

Douglas <ourairquality>
Sun 31 Dec 2017 01:16:20 PM UTC, comment #6: 

Let me phrase it differently. Up to now, there has been a more or less clean separation between "core" code (callback apis, single thread only) and the "API" layer (tcpip_thread, netconns, sockets, etc).

The only parts that broke this rule were some infrastructure code and snmp (don't know exactly why... :).

So your starting to use core locking for a callback style application does not really fit to lwIP the way it was. I'm not totally against such a change, but just adding it here would be confusing, I think (why would mdns work from other threads but other apps don't?). This would ultimately lead to adding core locking in all entry points to all modules (UDP, TCP, etc.).

Would it be too hard to write your application to call LOCK_TCPIP_CORE() before calling into mdns?

Core locking is already the default and message passing should be removed in one of the next releases, so you're OK with only implementing core locking code.

I wouldn't be too happy with requiring core locking to be recursive. Unless it makes it really hard for us to implement things, let's leave this as an option to the ones having to implement the lock.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 31 Dec 2017 11:46:19 AM UTC, comment #5: 


> Ehrm, callback api applications are single threaded by design. Without looking at your patch, I don't know what's meant by thread safety.


A 'pending' flag has been added for the announcement code paths which can be called from other threads as-is because most of the entry points into the mdns responder are not thread safe. The thread safe entry mdnsapi_mdns_resp_announce() has just been added, but the other entry paths are not protected. The patch also adds other entry functions that use the core lock for thread safety, to at least offer some safe proposal.

Some questions:

  • Do people want to have to implement netifapi style message passing code for every entry point? That's a lot of code and work to do in C.


  • Could people alternatively accept that in a threaded environment that core locking is required? This would allow a good deal of clean up work, would be faster, and make it easier to protect other entry points. What reasons would there be to not use core locking? Can the patch land as-is with only the use of core locking for safety?


  • Is it practical to also require the core lock to be recursive? This would simplify things further by avoiding the need to have duplicate entry points that are thread safe because possible entry points could take the recursive lock and could be called from the tcpip thread (recursive) or from other threads. Do people want all the duplicate entry points in this patch, or just to require a recursive core lock?
Douglas <ourairquality>
Sun 31 Dec 2017 10:44:12 AM UTC, comment #4: 


> Some thought also needs to be given
> to thread safety


Ehrm, callback api applications are single threaded by design. Without looking at your patch, I don't know what's meant by thread safety.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 31 Dec 2017 10:24:59 AM UTC, comment #3: 


>I don't really understand why this delay is need, but that appears to be talking about a reply to questions rather than the unsolicited announcements.


Right, I mixed it up. Sorry for the confusion!

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 31 Dec 2017 10:22:36 AM UTC, comment #2: 


> I'd rather like to fix the real problem - heavy stack usage by MDNS (aka bug #48953) instead of trying to hide the symptoms.
> You already provided a patch for it, but it needs more time to review and several cleanups before it can be applied.


Even with all the work the stack usage is still significant, and there are recursive algorithms that could spike the usage. I don't want every thread that simply wants to make a changes to settings to have to provision the extra stack, even if it's less than 1KB. Some thought also needs to be given to thread safety, so in part this patch is an attempt to discuss that too.

> But moving the announcement to a timer callback is a good idea, in combination with a pending flag and random delay it will also implement patch #9523 "the lightweight way".


I don't really understand why this delay is need, but that appears to be talking about a reply to questions rather than the unsolicited announcements. Deferring an answer would probably require very different changes, probably preparing the response and passing that to the timer callback to send later, or storing the questions. Whereas the current change just flags an announcement as required and it is built when the callback is called.

Douglas <ourairquality>
Sun 31 Dec 2017 09:40:46 AM UTC, comment #1: 

I'd rather like to fix the real problem - heavy stack usage by MDNS (aka bug #48953) instead of trying to hide the symptoms.
You already provided a patch for it, but it needs more time to review and several cleanups before it can be applied.

But moving the announcement to a timer callback is a good idea, in combination with a pending flag and random delay it will also implement patch #9523 "the lightweight way".

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 31 Dec 2017 07:33:25 AM UTC, original submission:  

The guts of the mdns code uses a significant amount of stack for a small system. This makes it problematic calling into this code from the various paths that trigger announcements. This patch has these code paths queue an announcement that will be performed via a timer callback.

The timer is used, rather than a tcpip_callback(), so that it will also work with NO_SYS. The slight delay might also help avoid redundant announcements when multiple changes are made quickly.

This patch also adds 'api' entry functions that use core locking to protect the mdns code state. This followed the recent example of mdnsapi_mdns_resp_announce(). This safety is currently only useful with core lock which it uses - the arp cache api added recently had this same limitation. Loath to try to add netifapi style callbacks for all of these, that is left as a TODO.

Do people have strong reasons for not making core lock mandatory when used in a threaded environment?

As the patch shows, many simple operations are much lighter using a lock rather that a callback.

Would also note that the separate api entry points might be redundant if a recursive lock were usable, and might that be a practical requirement to simplify the code?

Anyway, tried to work within all the constraints that were understood for this patch, and if it does not fit then feedback is welcomed?

Douglas <ourairquality>

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by ourairquality (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-04 goldsimon Open/ClosedOpen Closed
    2017-12-31 ourairquality Attached File- Added 0001-mdns-move-the-announcements-to-a-timer-callback.patch, #42770

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code