buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52763, mdns: move data off the stack and...

 
 

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

bug #52763: mdns: move data off the stack and dynamically allocate data.

Submitted by:  Douglas <ourairquality>
Submitted on:  Sat 30 Dec 2017 03:25:45 AM UTC  
 
Category:  DNS Severity:  3 - Normal
Item Group:  None Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

( Jump to the original submission)

Sun 07 Jan 2018 01:26:25 PM UTC, comment #16: 

Just a small update, fwiw.

  • Adds a strndup using mem_malloc. Still does not appear to address all the concerns and is still optimized for small memory usage.
  • Can still use a timeout to queue an announcement but defaults to zero delay. There seems little practical need for a delay.

One idea that might be food for thought:

  • Flush a pending async outpacket before sending or queuing any other packets. This both keeps them in order wrt state changes and minimizes the resource usage. Practically I don't see responses with more than one answer, so I expect they are rare. This strategy will fail if multiple responses are sent quickly, the delay might not be met, and that might happen if there are both ipv4 and ipv6 requests. If this is a problem then that queue could be increased from one, but still the idea would be to keep them in order via the queue and have a single timeout to flush the queue.

(file #42864)

Douglas <ourairquality>
Thu 04 Jan 2018 07:04:22 AM UTC, comment #15: 

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>
Project Administrator
Wed 03 Jan 2018 06:57:52 PM UTC, comment #14: 

Reading the lasts posts, we could probably close this bug as duplicate as we already have bug #48953 to remind us that stack usage in mdns is too high.

Douglas, I thought you had pools emulated via the heap, so using pools should be good for you (moving ram from the stack to the heap)?

>> LwIP does not use the C heap!
>
> Sounds like a barrier to attempts to manage the memory
> resource and with that barrier there seems no point in
> pursuing the mdns changes.


No, you're wrong there. You just can't use C library functions that directly use the heap. And there's no need for that, after all. You can still use a heap (via mem_malloc/mem_free).

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 03 Jan 2018 04:00:50 PM UTC, comment #13: 

> Adding even more unrelated code into the same patchset is not the way forward. Our time matters as well, and we want to make sure that the new code works. At some point it will be easier for me to just do the changes myself than to review this.


You might be able to solve the problem of moving the data off the stack, to reduce the peak stack usage, by putting the same static objects in a pool, but you will not reduce memory usage, and will have more ram taken for the pool.

> In your patch, the data allocated with strndup is returned with mem_free, but that will fail for everyone but you since the two are usually two totally different memory allocation systems.


I guess they would have to implement a variation of strndup that allocated from the same heap as mem_free.

I've leant a lot about lwip attempting this, and that is something to think about, but I'd like to take a break from this attempt now until I have some constructive ideas. Thank you for your help.

Douglas <ourairquality>
Wed 03 Jan 2018 03:45:32 PM UTC, comment #12: 

> Up to now, worst case calculation was deliberately used in lwIP to prevent "random" allocation failures when the heap gets empty.


Yes, I see that resource management strategy dominating lwip. On a limited device such guarantees can not be met, however with some resource management it might still be reliable enough for the job.

> LwIP does not use the C heap!


Sounds like a barrier to attempts to manage the memory resource and with that barrier there seems no point in pursuing the mdns changes.

> And regarding the separatoin of big changes into small commits: I also think it's important to have one issue solved per commit. That doesn't mean commits have to be small, but mixing things in one commit makes it really difficult to see what has change why when you look at it ten years later...


There is nothing constructive I can add, small commits are good, but I just don't have the resources.

Douglas <ourairquality>
Wed 03 Jan 2018 03:21:24 PM UTC, comment #11: 

Adding even more unrelated code into the same patchset is not the way forward. Our time matters as well, and we want to make sure that the new code works. At some point it will be easier for me to just do the changes myself than to review this.

lwIP already runs on platforms without any C standard library. Code that requires it will not be merged. The mdns code calls a common lwip_strnicmp() for example instead of any standard library.
In your patch, the data allocated with strndup is returned with mem_free, but that will fail for everyone but you since the two are usually two totally different memory allocation systems.

Simon, should we put the domain structs in a pool instead? I think we never need more than two. Is there any easy way to let the user configure if something is in its own pool or the common pool?

Keeping two domain structs in a pool uses the same RAM, but reserves it only for MDNS. That is why I kept it on the stack in the first place.

Erik Ekman <yarrick>
Project Member
Wed 03 Jan 2018 02:44:16 PM UTC, comment #10: 

Up to now, worst case calculation was deliberately used in lwIP to prevent "random" allocation failures when the heap gets empty. That's why we have pools. And you can use the heap to emulate the pools if you want to. However, I'm not comfortable with using mem_malloc all over the place. We have to let people configure the allocation scheme. You're the best example for that. But you can't enforce your allocaton scheme to all users of lwip. We have to find a way that fits all.

Regarding strndup: your comment about using it worries me a little. LwIP does not use the C heap! (Or only if you configure it to.) Strndup uses malloc, so it's a no-go!

And regarding the separatoin of big changes into small commits: I also think it's important to have one issue solved per commit. That doesn't mean commits have to be small, but mixing things in one commit makes it really difficult to see what has change why when you look at it ten years later...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 03 Jan 2018 02:10:46 PM UTC, comment #9: 

Thank you for the feedback, some of the issues caught are good to have fixed. The rebased patch combines all the fixes, even the reverted support for delaying responses. I doubt there are the resources to slice this all up sorry, so could we try to move forward.

> mdns_service_free() has a copy-paste error:


Good catch, glad that one was fixed, thanks.

> I don't really see the point of keeping the host name in a separately allocated block. Its lifetime is the same as the surrounding struct, and it only makes a few bytes difference. Also, the name might have to be adjusted so it is good to know the amount of space available. This also applies to the char arrays in the service struct.


It uses less ram storage space in most cases. This patch is largely about moving from all the statically allocate maximum sized storage blocks to variable length allocations. For a variable size it can not be a part of a statically sized C struct, so it's a C string pointer. I understand the change takes an almost opposite style of allocation, and the original code style is probably better if ram is plentiful, but this patch is about optimizing for low ram usage rather than performance and code size.

> You cannot count on any string functions being available, especially strndup which does an allocation. lwIP must not have any dependencies outside of what the port is expected to provide.


strndup is a standard C library string function so it seems reasonable to expect it to be provided. If some port does not have it seems better for them to just add it in port specific code rather than requiring all ports to duplicate a standard function. Looking at that code again there was a lack of error checking on these which has been added.

> Please update the function comments in the cases where you change the method signature.


Done, forget those, thanks.

> The late declaration of err_t res will not compile on stricter systems.


Yes, good point, fixed. Seem to keep making that mistake.

> The old mdnsapi_* functions are still in the header file.


Opps, fixed.

> Please split out the LWIP_MDNS_RESPONDER_QUEUE_ANNOUNCEMENTS changes into a separate patch.


There seems to be interactions between a lot of these changes, so it seems best to consider how it all fits together. The patch combines fixes for all the issues raised. It's a lot of work to start breaking things up. I understand it's a bigger job to review together, but my time is also limited.

> Does this function still compile without the int declaration?
> mdns_resp_remove_netif(struct netif *netif)
> {
> - int i;


I don't see that in the current patch.

> The dns_hdr struct is 12 bytes, that should be small enough to keep it on the stack.


This patch is about moving data off the stack. I have other reasons for wanting a struct or array off the stack, for checking accesses. Only trivial data types remain on the stack.

The updated patch also adds a random delay for announcements, computing them after the delay. It also adds some lock assertions.

(file #42805)

Douglas <ourairquality>
Wed 03 Jan 2018 12:42:59 PM UTC, comment #8: 

Sorry, we reverted the async send commit, so your patch is out of date again.

mdns_service_free() has a copy-paste error:
 if (service) {
   mem_free(name);
 }

I don't really see the point of keeping the host name in a separately allocated block. Its lifetime is the same as the surrounding struct, and it only makes a few bytes difference. Also, the name might have to be adjusted so it is good to know the amount of space available. This also applies to the char arrays in the service struct.

You cannot count on any string functions being available, especially strndup which does an allocation. lwIP must not have any dependencies outside of what the port is expected to provide.

Please update the function comments in the cases where you change the method signature.

The late declaration of err_t res will not compile on stricter systems.

The old mdnsapi_* functions are still in the header file.

Please split out the LWIP_MDNS_RESPONDER_QUEUE_ANNOUNCEMENTS changes into a separate patch.

Does this function still compile without the int declaration?
 mdns_resp_remove_netif(struct netif *netif)
 {
-  int i;

The dns_hdr struct is 12 bytes, that should be small enough to keep it on the stack.

Erik Ekman <yarrick>
Project Member
Wed 03 Jan 2018 03:16:18 AM UTC, comment #7: 

Rebased around the other improvements, and thank you for the help.

Also removed some dead code to silence a compiler warning.

The patch also adds the option LWIP_MDNS_RESPONDER_QUEUE_ANNOUNCEMENTS, which defaults to 0, to help assist limited systems that prefer to queue the announcements, and this uses a timer callback with a delay of zero.

(file #42793)

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

> Please check if test/unit/mdns/test_mdns.c needs to be updated as well - you can run it from ports/unix/check in the contrib repo.


Thank you for the feedback. The test did need updating, and found many style problems which have been corrected. The thread safe entry points have also been removed.

(file #42773)

Douglas <ourairquality>
Sun 31 Dec 2017 01:19:09 PM UTC, comment #5: 

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


If I remember correctly, the intention was to be able to leave it out completely if and only if you are absolutely sure your application will never trigger this error handling path. Someone back then was under the impression that removing those statements could save just the bytes of flash he needed, I guess... :-/

Simon Goldschmidt <goldsimon>
Project Administrator
Sun 31 Dec 2017 12:38:51 PM UTC, comment #4: 

Nice to see progress. Some more comments:

For LWIP_ERROR, I think this is the minimal definition:

#define LWIP_ERROR(message, expression, handler) do { if (!(expression)) { handler; }} while(0)

Asserts and the logging message can be ignored, but not the handler as I understand it.

realloc isnt supported by the lwIP memory abstraction, it was renamed mem_trim since it could only shrink the allocation - so we needed to stop using realloc anyway.

Please check if test/unit/mdns/test_mdns.c needs to be updated as well  - you can run it from ports/unix/check in the contrib repo.

You don't need the semicolon after the 'err:' label.

Erik Ekman <yarrick>
Project Member
Sun 31 Dec 2017 10:09:16 AM UTC, comment #3: 

Thank you for all the feedback, and an attempt has been made to address it all.

> Maybe having just a goto in the LWIP_ERROR macro and freeing the domain and return in a common block?


Done, this now assumes that the LWIP_ERROR always includes the handler forms otherwise it will give unreachable code warnings. I had not understood that LWIP_ERROR should have done that and had defined it to be empty in non-debug code, but perhaps that was just my mistake? Was the intention for the LWIP_ERROR check to always be performed, but perhaps only verbose in debug builds?

> Running realloc on each adding of a name to a domain seems like it will fragment memory very quickly - or maybe many of the realloc calls will need to do nothing if more memory than requested is given earlier.


There are trade offs, and stack allocation is fast and simple, but it is not easy to grow a stack allocation and allocating the peak usage seems excessive for a very limited system.

The use of realloc() has been removed for now, it uses mem_malloc() and mem_free().

(file #42771)

Douglas <ourairquality>
Sat 30 Dec 2017 03:13:15 PM UTC, comment #2: 

In mdns_domain_alloc(), mem_calloc already clears the memory so the memset is not required.

Same in mdns_question_alloc() and mdns_answer_alloc().

Erik Ekman <yarrick>
Project Member
Sat 30 Dec 2017 03:07:20 PM UTC, comment #1: 

Can we simplify the cleanup code in the build domain functions?
Maybe having just a goto in the LWIP_ERROR macro and freeing the domain and return in a common block?

Running realloc on each adding of a name to a domain seems like it will fragment memory very quickly - or maybe many of the realloc calls will need to do nothing if more memory than requested is given earlier.

I have not used lwIP with locking so I have no comments there.

Erik Ekman <yarrick>
Project Member
Sat 30 Dec 2017 03:25:45 AM UTC, original submission:  

The mdns responder uses a lot of stack for a small system, well over 1k, which is out of proportion to the general stack resource needs for a small system. This is largely due to stack allocated fixed sized 256 byte buffers for domain names. This patch largely moves data off the stack and dynamically allocates it.

The code also allocated fixed size buffers for the domain names, big enough for the maximum size name of 256 bytes. This patch dynamically allocates buffers just large enough for their content, growing them as needed. There are trade offs in doing so, but with this change it is still trivial to over allocate storage if that is preferred.

The patch currently uses realloc() to grow the buffers. Some compatibility code is expected to be needed to fit other systems.

Some context: on the esp8266 using lwip with FreeRTOS the large stack usage is problematic, it demands almost an extra 1k be allocated to the stack, and there is little ram to spare for some apps. Another issue is that instruction ram can be used as data ram as a last resort but this iram has restrictions and best not to put a task stack there making stack allocation less attractive.

The patch bundles in the change suggested in https://savannah.nongnu.org/bugs/?52747 which attempts to avoid the mdns code being called in other threads (it still uses a bit of stack). Not really happy with that solution as it also required core locking to be disabled. Perhaps there is a need for some other queue for jobs to run in the tcp thread when it is free, to avoid a resource spike, while still supporting core locking.

This is a WIP, feedback welcomed.

Douglas <ourairquality>

 

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 yarrick (Posted a comment)
  • -email is unavailable- added by ourairquality (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
    2018-01-07 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42864
    2018-01-04 goldsimon Open/ClosedOpen => Closed
    2018-01-03 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42805
    2018-01-03 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42793
    2017-12-31 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42773
    2017-12-31 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42771
    2017-12-30 ourairquality Attached File- => Added 0001-mdns-move-data-off-the-stack-and-dynamically-allocat.patch, #42761

    Back to the top


    Powered by Savane 3.5