buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52747, mdns resp: separate the announce...

 
 

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

bug #52747: mdns resp: separate the announce function and add netifapi support for it

Submitter:  Douglas <ourairquality>
Submitted:  Wed 27 Dec 2017 01:31:37 PM UTC
   
 
Category:  DNS Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Sat 30 Dec 2017 10:03:18 PM UTC, comment #6: 


> Well, not exactly maybe. That would break layering. mdns should work as callback app without tcpip thread support, too...


It seems to work well using 'tcpip_callback()', and with a 'pending' flag it can even avoid some redundant announcements.

Could 'tcpip_callback()' be extended to work in the NO_SYS case too, in a similar way that the timers are currently checked by calling 'sys_check_timeouts()' in the event loop?

These callback might be useful even for NO_SYS as this might defer a heavy operation and this might help with latency for some operations and help reduce the peak resource usage. In any case it might be a small burden for NO_SYS that helps keep the large code cleaner.

Douglas <ourairquality>
Sat 30 Dec 2017 02:14:44 PM UTC, comment #5: 

Well, not exactly maybe. That would break layering. mdns should work as callback app without tcpip thread support, too...

Simon Goldschmidt <goldsimon>
Group administrator
Sat 30 Dec 2017 01:40:43 PM UTC, comment #4: 


> You mean something like 'tcpip_callback()'?


Yes, thank you for the pointer. Let me try to patch mdns to use 'tcpip_callback()' for the announcements.

Douglas <ourairquality>
Sat 30 Dec 2017 01:10:21 PM UTC, comment #3: 


> What would people think about having a
> queue for tasks to be run in the tcp
> thread.


You mean something like 'tcpip_callback()'?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 30 Dec 2017 12:52:33 PM UTC, comment #2: 

A significant part of the change was to stop announcing on calls to mdns_resp_add_netif and mdns_resp_add_service, see "The interface setup functions no longer invokes an announcement."

There is currently no safe way to call these setup functions from the tcp thread, and as-is it calls into the guts of the mdns code in the context of the caller and that requires all these threads provision for the significant stack allocation requirements of the mdns code. This was the problem trying to be solved, and remains to be solved.

What would people think about having a queue for tasks to be run in the tcp thread. Then the announcements could be queued to run there. This same approach could be used for similar challenges. This could work alongside core locking, so core locking could be used for light weight functions that can be run in the caller thread context and if they trigger any heavier weight code then they could queue that to run in the tcp thread.

The core locking could then be used for these mdns setup function, and if they are to trigger an announcement then that could be queued.

From other discussions it appears that core locking might become mandatory, yet there might still be a need to be able to queue tasks to run in the tcp thread just because they are too heavy to run in the caller context.

Douglas <ourairquality>
Sat 30 Dec 2017 11:08:49 AM UTC, comment #1: 

Applied a modified version of your patch. Thanks!

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 27 Dec 2017 01:31:37 PM UTC, original submission:  

The mDNS responder uses a lot of stack for a small system. This patch attempts to mitigate this by allowing the announce code paths to run in the tcpip thread where the stack can be provisioned, and removing that burden from all threads that might invoke an announcement.

The interface setup functions no longer invokes an announcement. Typically a series of calls are used to setup the state and it is only useful to send an announcement at the end anyway. The caller is now responsible for invoking an announcement as needed after making changes.

Netifapi support is added for invoking an announcement. This was trivial to add as it takes only the netif as an argument. It is left to future work to add other api support for the mDNS responder.

The strategy only works when core locking is not used, otherwise the code paths can be invoked in other threads that make interface changes and they would all need to provision for the excessive stack usage.

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 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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-30 dziegel StatusNone Fixed
        Open/ClosedOpen Closed
    2017-12-28 ourairquality Attached File- Added 0001-mdns-resp-separate-the-announce-function-and-add-net.patch, #42745
    2017-12-27 ourairquality Attached File- Added 0001-mdns-resp-separate-the-announce-function-and-add-net.patch, #42735

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code