patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9523, MDNS responder should reply after...

 
 

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

patch #9523: MDNS responder should reply after a random timeout

Submitter:  David GIRAULT <dgirault>
Submitted:  Tue 19 Dec 2017 01:39:44 PM UTC
   
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Wed 14 Nov 2018 08:18:02 AM UTC, comment #28: 

Hi Axel,

You will indeed find some commit logs in "one" commit because that commit originates from multiple commits. For example commit: "mDNS: add support for output delaying" is such a big change that during development I made several commits.
But as asked by Simon, I interactively rebased the commits so I can send fewer commits to you. The commit messages are kept for clarity on what the commit all changes.

I made the revert from my development environment, not from the rebased commit history, sorry :). I will take this in account next time.
Can you apply it?
If you can't, maybe it's easier if you just revert the commit.

Thanks for pushing!

Jasper Verschueren <jasperv>
Tue 13 Nov 2018 01:59:35 PM UTC, comment #27: 

Hi Jasper,

In below commit, I cannot find the e959e6efaf8a1cb57e9a5d4d8e0056c7ea52179f commit.
I think it's better to mention correct commit in lwIP tree.

commit 405fc4d8026a25f1f16cdd7fd9e7f45fc8b8eac9
Author: Jasper Verschueren <jasper.verschueren@apart-audio.com>
Date:   Fri Nov 9 17:10:40 2018 +0100

    Revert "mDNS: authority section of probe message needs to contain all records"
   
    This reverts commit e959e6efaf8a1cb57e9a5d4d8e0056c7ea52179f.

In additonal, I also found the commit log of below commit includes
a lot number of commits which are not in lwip tree which is very strange.

commit 7d1c26cc0c18d02cc72f348f9189dd1c3a06bfca
Author: Jasper Verschueren <jasper.verschueren@apart-audio.com>
Date:   Thu Sep 13 17:02:21 2018 +0200

Axel Lin <axellin>
Group Member
Tue 13 Nov 2018 11:24:48 AM UTC, comment #26: 

Pushed your patches. Thanks you very much for your contributions!

Dirk Ziegelmeier <dziegel>
Group administrator
Mon 12 Nov 2018 08:59:29 AM UTC, comment #25: 

I'm currently working on the probe tiebreaking and found that I made a small mistake with commit:
mDNS: authority section of probe message needs to contain all records.

It does not need to contain all records, only the records that answer the probe questions. Attached you can find the revert of the commit.

We probe for the hostname.local. the A and the AAAA record answer this.
We also probe for the hostname.service.transportprot.local. the SRV and the TXT answer that domain.

So we do not need to add any of the PTR's.

Any comments yet on the code?

(file #45412)

Jasper Verschueren <jasperv>
Thu 01 Nov 2018 09:13:03 PM UTC, comment #24: 

netif_mdns_data() and get_mdns_pcb() should go to priv header. The function to get mdns pcb can go away if mdns_out.c would only contain the routines to build a packet and mdns.c contains the routine to send a packet.

But these "issues" are totally minor, they can always be easily fixed/refactored later!

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 31 Oct 2018 04:05:50 PM UTC, comment #23: 

Response to comment #21:
ok. Do you agree with netif_mdns_data() to get the mdns data from the netif in the other modules?

Response to comment #22:
I need the busy_flag anyway to keep track of the status. So looking at the busy_flag instead of going through a linked list seems more appropriate to me.
The abstraction level that mdns_set_timeout makes is convenient. It also updates the busy_flag.

I have MEMP_NUM_SYS_TIMEOUT of 18 at this moment, so worst case it will need to run through a linked list of 18 timers...

Jasper Verschueren <jasperv>
Wed 31 Oct 2018 11:32:07 AM UTC, comment #22: 

Do you really need mdns_set_timeout()? AFAICS, you can always call sys_untimeout() even if the timeout is not active.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 31 Oct 2018 09:58:56 AM UTC, comment #21: 

Regarding your question about NETIF_TO_HOST: Only lwIP CORE functionality should be a direct type-safe netif member. This is because lwIP core should not know about apps, netifs etc., even if they are in main lwIP rep. This is why all apps that need to store something on a netif need to use the netif_client_data mechanism.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 31 Oct 2018 09:53:00 AM UTC, comment #20: 

No problem at all. I indeed thought you wanted only one patch :).

I rebased my commits a little and tried to combine as much as possible. I believe that this should be good, if not, I can still send you all patches of my commits. They gradually add functionality without removing and changing a lot along the way. So on their own they are quite clear.

You can find the rebased patches attached (in a zip).

I will try to combine patches in logical steps next time too :).

(file #45336)

Jasper Verschueren <jasperv>
Tue 30 Oct 2018 09:38:37 PM UTC, comment #19: 

I'm happy someone's working on this!

But the patch is hard to review since it is very big and contains code restructuring as well as new code.

I know I said "post a single patch, not multiple" last time, but what I meant is something different. To review a patch, it's best to separate it into smaller commits that show different logical steps, not just your history of how you did what and when.

I.e. it would greatly speed up the review process if you could provide a patchset that e.g. first has all the code changes and then separates the files (or the other way round). And ideally another patch (as the first in the series) that fixes bugs in the existing code.

I can well understand if this is too much work for you, but this is how good patches should look like. This is inspired by the Linux way, yes, and lwIP may not keep up with this, but still: I'll try to find the time to review this before pushing. Getting this input from you will speed up the process.

Thanks again for sharing your work.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 30 Oct 2018 10:27:40 AM UTC, comment #18: 

The approach suggested below is not perfect. To implement the random delays we need to transform the code a bit further. With our mind on the other stuff that needs to be implemented.

I tried to do that, you can find the patch attached.
The mdns.c started to get very big, that is why I made 2 extra modules. mdns_domain.c contains all domain related generation functions. These are utility functions for the other to mDNS modules. mdns_out.c contains all the output functionality. A clean split is made between defining what the packet should be and generating the blob of data to be send out. mdns_out contains a lot of functionality only needed to generate the packet. These are now separated from the other modules. Only a few functions are needed to be global to the other mdns modules.

I worked with sys_timeout to make the random timeouts possible. Because a lot of different timeouts are needed, you need to increase the MEMP_NUM_SYS_TIMEOUT with 8.

I solved some smaller bugs with for example TTL along the way. They needed to be correct to test the full functionality of the delaying. Also the source address check is added.

The RFC did not make it easy to implement ... You should mainly read section 6 Responding to find the why of things.

One question about NETIF_TO_HOST function: why can't we add the module to the standard netif client data like we do for autoip, dhcp,...? I need it to be accessible in all modules and that would make it cleaner.

Please let me know your comments.
 


(file #45329)

Jasper Verschueren <jasperv>
Tue 16 Jan 2018 08:46:58 PM UTC, comment #17: 

The only thing I have to add here currently is: please don't add cyclic timers below 100ms. There are targets where this might hurt. Especially in the 5ms range (in fact, the 100ms cyclic timers already seem too fast for doing near nothing).

Instead, calculating a random time and passing this to sys_timeout() would be better.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 16 Jan 2018 07:09:05 PM UTC, comment #16: 

Even if many units happen to send at the same time, Ethernet takes care of that for you - either way it will be much better than now. I am happy to discuss such details once the rest of the code is in place.

Do you think the suggested direction is reasonable in general?

Erik Ekman <yarrick>
Group Member
Tue 16 Jan 2018 04:24:20 PM UTC, comment #15: 

I'm not sure that assuming different devices will have unsynchronized periodic timer will be ok. If sys_now() implementation is base on gettimeofday() for example (well I know, this is a bad thing), and this one in keep sync using NTP, this result in periodic timer sync across all devices.

Personally, I prefers a TRUE random time against request reception. A faster on-demand tick (5ms) with a random counter may do the job anyway.

David GIRAULT <dgirault>
Mon 08 Jan 2018 09:13:50 PM UTC, comment #14: 

As I understand it, there is no concept of order in MDNS.
The responder is free to merge multiple different multicast (and non-legacy unicast) into a single response packet sent as multicast.

This reduces the load on the network, as well as makes multiple packet known address suppression easy. When a service is removed, all outgoing queued replies for it should be cleared. The legacy question would have to be temporarily copied (just the RR info: domain, class and type)

My suggestion is to have a 50ms or so tick and wait one or two ticks for instance before sending a certain reply. That should be random enough since different devices will not have a synchronized periodic timer. We could also use this tick to make sure to not send an answer again if we sent it multicast during the last second. (RFC 6762, section 6) as well as for driving the collision probing logic.

Erik Ekman <yarrick>
Group Member
Mon 08 Jan 2018 01:30:32 PM UTC, comment #13: 

Douglas, regarding your comment #8, I think it should be randomized even for unicast response. If not, this won't resolve our issue when a device request (with unicast) for a service and multiple device responds at exactly same time (same HW, same code, so same reaction time). Most of the time, 1 or 2 of 4 devices aren't seen in our case.

David GIRAULT <dgirault>
Mon 08 Jan 2018 11:56:58 AM UTC, comment #12: 

Erik, what's your status? Do you want to work on this or should someone else do it?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Jan 2018 10:15:30 AM UTC, comment #11: 

I think the approach is good, that is building the output pbuf and holding it for a short delay. I tried other options such as building the pbuf only when sending and it did not fit well, it had the problem that the response was not built atomically (the services might change), and for legacy queries the questions need to be added. If no one else is doing a major rewrite then landing this seems a move forward.

Douglas <ourairquality>
Mon 08 Jan 2018 09:35:30 AM UTC, comment #10: 

I've made a new version with a flag that allow to force synchronous send when mdns_send_outpacket() called from mdns_announce() or mdns_search(). See new patch.

(file #42869)

David GIRAULT <dgirault>
Thu 04 Jan 2018 12:45:54 PM UTC, comment #9: 

Also should the responses still be sent in order. With only the random delay they might end up being sent out of order.

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

Perhaps the delay should only be applied for multicast, so the test in mdns_send_outpacket() would become:

    /* Delayed answer? See 6.3 in RFC 6762. */
    if (outpkt->unicast_reply == 0 && outpkt->answers > 1) {
      struct mdns_async_outpacket *async_outpkt;
      ...


Douglas <ourairquality>
Wed 03 Jan 2018 09:29:50 AM UTC, comment #7: 


> Note the patch also delays announcements with a random delay. This may not be a bad thing, if several devices of the same type power up at the same time, this avoids floodeding the net with announcements.


If the announcements are to be delayed too then it seems best to build the response in future as this saves holding that resource (briefly) and gives a chance to combine multiple triggered announcement. If a few announcements were triggered quickly then there could be a few output packets queued with this patch.

The proposed LWIP_MDNS_RESPONDER_QUEUE_ANNOUNCEMENTS will queue the generation of the announcement and could add a random delay.

Perhaps an argument could be added to mdns_send_outpacket() to flag that the output is to be sent immediately, for the announcement path, to avoid a double delay.

Douglas <ourairquality>
Wed 03 Jan 2018 08:20:12 AM UTC, comment #6: 

Note the patch also delays announcements with a random delay. This may not be a bad thing, if several devices of the same type power up at the same time, this avoids floodeding the net with announcements.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 03 Jan 2018 06:51:11 AM UTC, comment #5: 

OK, I reverted the patch. Erik, may I ask you to take over the current work on MDNS? You seem to be interested :-)

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 03 Jan 2018 02:04:52 AM UTC, comment #4: 

It seems this broke the build.

Also, I didn't think it was ready to merge yet. There are also other changes going on for mdns, I think this would be easier to do after the move of things from the stack.

Erik Ekman <yarrick>
Group Member
Tue 02 Jan 2018 09:21:07 PM UTC, comment #3: 

Applied, thanks!

Dirk Ziegelmeier <dziegel>
Group administrator
Sat 23 Dec 2017 07:17:52 PM UTC, comment #2: 

Ehrm, I haven't looked at the patch, nor do I know the mdns code too well, but RFC 6762, chapter 6.3 includes this:
"all (non-defensive) answers SHOULD be randomly delayed in the range 20-120 ms, or 400-500 ms if the TC (truncated) bit is set."
So I do think the OP does have a point. I can't speak for the contents of the patch (yet).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Dec 2017 10:08:28 AM UTC, comment #1: 

Thanks for the patch.

The RFC only states that query messages containing more than one question SHOULD be delayed a bit. Not MUST - and multiple questions, not answers.

This feels like quite a bit of duplication of code. I would prefer also delaying the creation of the packet.
That would mean splitting mdns_outpacket into the parts the needed to know what packet to generate, and the parts needed while generating the packet (like fields: pbuf, questions, answers, additional, domain_offsets) into separate structs.

This can then also be used for multi-packet known address suppression, by clearing some reply bits if a match is received while waiting to send.

The code for sending a packet immediately or later would then only be the decision of when to convert the struct with the reply bits to the one with the pbuf.

Erik Ekman <yarrick>
Group Member
Tue 19 Dec 2017 01:39:44 PM UTC, original submission:  

According to chapter 6.3 in RFC 6762, the reply to a query that result in multiple answers must be randomly delayed from 20 to 120ms.

This patch add support for this requirement.

David GIRAULT <dgirault>

 

(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 axellin (Posted a comment)
  • -email is unavailable- added by jasperv (Updated the item)
  • -email is unavailable- added by ourairquality (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by yarrick (Posted a comment)
  • -email is unavailable- added by dgirault (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-13 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2018-11-12 jasperv Attached File- Added 0001-Revert-mDNS-authority-section-of-probe-message-needs.patch, #45412
    2018-10-31 jasperv Attached File- Added 2018-10-31_mDNS_responder_delayed_refactored.zip, #45336
    2018-10-30 jasperv Attached File- Added 0001-mDNS-responder-upgraded-with-delay-functionality-cle.patch, #45329
    2018-01-08 dgirault Attached File- Added async_mdns_response.patch, #42869
    2018-01-03 dziegel StatusDone None
        Open/ClosedClosed Open
    2018-01-02 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2017-12-19 dgirault Attached File- Added lwip_mdns_async_reply.patch, #42682

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code