[patch #9523] MDNS responder should reply after a random timeout

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
URL:
  <http://savannah.nongnu.org/patch/?9523>

                 Summary: MDNS responder should reply after a random timeout
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: dgirault
            Submitted on: Tue 19 Dec 2017 01:39:44 PM UTC
                Category: apps
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

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.




    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Tue 19 Dec 2017 01:39:44 PM UTC  Name: lwip_mdns_async_reply.patch
Size: 4KiB   By: dgirault

<http://savannah.nongnu.org/patch/download.php?file_id=42682>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #1, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #2, patch #9523 (project lwip):

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).

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Update of patch #9523 (project lwip):

                  Status:                    None => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #3:

Applied, thanks!

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #4, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Update of patch #9523 (project lwip):

                  Status:                    Done => None                  
             Open/Closed:                  Closed => Open                  

    _______________________________________________________

Follow-up 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 :-)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #6, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #7, patch #9523 (project lwip):

> 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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #8, patch #9523 (project lwip):

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;
      ...



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #9, patch #9523 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #10, patch #9523 (project lwip):

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)
    _______________________________________________________

Additional Item Attachment:

File name: async_mdns_response.patch      Size:4 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #11, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #12, patch #9523 (project lwip):

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

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #13, patch #9523 (project lwip):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #14, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #15, patch #9523 (project lwip):

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.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #16, patch #9523 (project lwip):

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?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #17, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #18, patch #9523 (project lwip):

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)
    _______________________________________________________

Additional Item Attachment:

File name: 0001-mDNS-responder-upgraded-with-delay-functionality-cle.patch
Size:152 KB


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #9523] MDNS responder should reply after a random timeout

Jasper Verschueren
Follow-up Comment #19, patch #9523 (project lwip):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9523>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel
12