From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin KaFai Lau Subject: Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket Date: Tue, 22 Mar 2016 10:39:39 -0700 Message-ID: <20160322173939.GA53936@kafai-mba.local> References: <20160316.223803.240230703927987310.davem@davemloft.net> <1458185652.7353.28.camel@edumazet-glaptop3.roam.corp.google.com> <20160322060220.GA50824@kafai-mba.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: Wei Wang , Wei Wang , Eric Dumazet , David Miller , Eric Dumazet , Linux Kernel Network Developers To: Cong Wang Return-path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:11527 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751212AbcCVRju (ORCPT ); Tue, 22 Mar 2016 13:39:50 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Mar 22, 2016 at 09:53:35AM -0700, Cong Wang wrote: > On Mon, Mar 21, 2016 at 11:02 PM, Martin KaFai Lau wrote: > > In term of difference, AFAICT, the current patch is an optimization in the > > sense that the update_pmtu() code path does not have to do a dst_check to > > discover its sk->sk_dst_cache is invalid, and then do a relookup to find out > > that the just created RTF_CACHE clone should be used. To get this, it may > > make more sense to remove all the relookup code together during update_pmtu(). > > Even if this slow path was to be optimized, should it be put in a > > separate patch where net-next is a better candidate? > > > > Speaking of RTF_CACHE, I am curious why you didn't use FIB next hop exception > as what ipv4 does to cache exceptions? This makes IPv6 has more gap with IPv4. > This is (almost) irrelevant to this patch. There are a few differences between IPv6 and IPv4. Both in terms of data structure and functionality. The last 'RTF_CACHE on exception' patchset is one step toward this direction. More patches are needed and are welcomed ;) > > > > I think fixing it in __udp6_lib_err() or what Cong Wang is suggesting makes > > more sense for a net branch fix. If there is logic specific to connected-udp, > > I would do it in the __udp6_lib_err() instead. After looking at > > udpv6_sendmsg() and how it calls ip6_dst_store(), may also need to be careful > > what daddr and saddr should be passed to ip6_dst_store(), or at least a commit > > message. The first patch is essentially passing NULL to daddr and saddr > > while the second patch seems passing something else. > > Raw socket needs to fix too, we can't just fix __udp6_lib_err(), this is also > why fixing ip6_sk_update_pmtu() is better, its call path is better. I don't see rawv6 socket is storing the dst. I probably have overlooked it. Can you point it out? Having said that, I don't feel strongly on any of the two places. I think only implementation can tell.