On 4/14/23 04:15, Han Zhou wrote: > On Thu, Apr 13, 2023 at 12:44 PM Han Zhou <[email protected]> wrote: >> >> >> >> On Thu, Apr 13, 2023 at 12:35 PM Dumitru Ceara <[email protected]> wrote: >>> >>> 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? >>> >> Sound good. I will do that. > > I backported this series to the last release branch-23.03 and the LTS > branch-22.03. > The code base has changed a lot so was mostly manually backported instead > of cherry-pick. >
Thanks for that but I think we shouldn't skip stable branches in between. We likely still have users on those that won't upgrade to the latest 23.03 but instead to a new z-stream release (when that happens) of the stable branch. I cherry picked your backports and made sure the tests pass and then pushed them to branches 22.12, 22.09 and 22.06. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
