On 4/13/23 21:21, Han Zhou wrote: > On Thu, Apr 13, 2023 at 8:37 AM Dumitru Ceara <[email protected]> wrote: >> >> On 4/13/23 17:34, Han Zhou wrote: >>> On Thu, Apr 13, 2023 at 12:54 AM Dumitru Ceara <[email protected]> > wrote: >>>> >>>> On 4/13/23 07:07, Han Zhou wrote: >>>>> In RFC1812 section 5.3.1, it is mentioned that: >>>>> >>>>> If the TTL is reduced to zero (or less), the packet MUST be >>>>> discarded, and if the destination is not a multicast address the >>>>> router MUST send an ICMP Time Exceeded message ... >>>>> >>>> >>>> The code itself looks OK but I wonder a bit about the rationale. Do > you >>>> have an example in which OVN replies with Time Exceeded for multicast >>>> destinations and that causes issues? >>>> >>>>> So if the destionation is a multicast address the route shouldn't send >>>>> ICMP Time Exceeded, but the current OVN implementation didn't check >>>>> multicast and tries to send ICMP regardless. This patch fixes it. >>>> >>>> The statement "if destination is not a multicast address the router > MUST >>>> send an ICMP Time Exceeded message" implies that "if destination is a >>>> multicast address the router MAY or MAY NOT send an ICMP Time Exceeded >>>> message". So the fact that OVN sends one is not necessarily wrong. >>> >>> For my limited understanding of multicast, sending ICMP time exceeded is >>> not a good idea. In multicast TTL has special meanings, for example: >>> TTL 0: Restricted to the same host, not transmitted by the router. >>> TTL 1: Restricted to the same subnet, not forwarded by the router. >>> If we send ICMP for such packets, it means for a very common use case of >>> multicast (ttl = 1, same subnet), we will end up sending ICMP for every >>> normal packet. >>> In production we saw this happening with a rate higher than 10k pps! >>> >> >> Makes sense, thanks for the details! >> >>> So I believe this is the reason behind the "if" statement in the RFC. > Maybe >>> I should add this rationale in the comment, too. >>> >> >> If you could add some of the details above to the commit log too then: >> >> Acked-by: Dumitru Ceara <[email protected]> > > Thanks Dumitru. I applied the series to main. > Han
This sounds like a relevant bug fix. Should this get backported to stable branches too? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
