All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guillaume Nault <gnault@redhat.com>
To: Hangbin Liu <liuhangbin@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org, ja@ssi.bg, marcelo.leitner@gmail.com,
	dsahern@gmail.com, edumazet@google.com
Subject: Re: [PATCHv2 net] ipv6/route: should not update neigh confirm time during PMTU update
Date: Tue, 10 Dec 2019 18:00:00 +0100	[thread overview]
Message-ID: <20191210170000.GA1132@linux.home> (raw)
In-Reply-To: <20191210033656.GM18865@dhcp-12-139.nay.redhat.com>

On Tue, Dec 10, 2019 at 11:36:56AM +0800, Hangbin Liu wrote:
> Hi David,
> 
> Sorry for the late reply. Hope you still have impression for this discussion.
> I discussed this issue with my colleagues offline and I still have some questions.
> Please see comments below.
> 
> On Tue, Dec 03, 2019 at 11:58:18AM -0800, David Miller wrote:
> > >> > That's not what I said.
> > >> > 
> > >> > I said that this interface is designed for situations where the neigh
> > >> > update is appropriate, and that's what happens for most callers _except_
> > >> > these tunnel cases.
> > >> > 
> > >> > The tunnel use is the exception and invoking the interface
> > >> > inappropriately.
> > >> > 
> > >> > It is important to keep the neigh reachability fresh for TCP flows so
> > >> > you cannot remove this dst_confirm_neigh() call.
> 
> The first is why IPv4 don't need this neigh update. I didn't
> find dst_confirm_neigh() or ipv4_confirm_neigh() in ip_rt_update_pmtu()
> 
> > > 
> > > I have one question here. Since we have the .confirm_neigh fuction in
> > > struct dst_ops. How about do a dst->ops->confirm_neigh() separately after
> > > dst->ops->update_pmtu()? Why should we mix the confirm_neigh() in
> > > update_pmtu(), like ip6_rt_update_pmtu()?
> > 
> > Two indirect calls which have high cost due to spectre mitigation?
> 
> Guillaume pointed me that dst_confirm_neigh() is also a indriect call.
> So it should take same cost to call dst_confirm_neigh() in or before
> __ip6_rt_update_pmtu(). If they are the same cose, I think there would
> have two fixes.
> 
OTOH, the dst_confirm_neigh() call could easily be replaced by a direct
ip6_confirm_neigh() call in the current code (maybe using an
INDIRECT_CALL wrapper if necessary).
I'm not sure where dst_confirm_neigh() would go if it was moved outside
of __ip6_rt_update_pmtu(), but that might make such optimisation
harder.

> 1. Add a new parameter 'bool confirm_neigh' to __ip6_rt_update_pmtu(),
> update struct dst_ops.update_mtu and all functions who called it.
> 
> 2. Move dst_confirm_neigh() out of __ip6_rt_update_pmtu() and only call it
>    in fuctions who need it, like inet6_csk_update_pmtu().
> 
> What do you think? Please tell me if I missed something.
> 
> Regards
> Hangbin
> 


  reply	other threads:[~2019-12-10 17:00 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-22  6:19 [PATCH net] ipv6/route: only update neigh confirm time if pmtu changed Hangbin Liu
2019-11-22 18:04 ` David Miller
2019-11-26  9:17   ` Hangbin Liu
2019-12-03  2:11 ` [PATCHv2 net] ipv6/route: should not update neigh confirm time during PMTU update Hangbin Liu
2019-12-03  2:47   ` David Miller
2019-12-03 10:15     ` Hangbin Liu
2019-12-03 10:25       ` Hangbin Liu
2019-12-03 19:58         ` David Miller
2019-12-10  3:36           ` Hangbin Liu
2019-12-10 17:00             ` Guillaume Nault [this message]
2019-12-18 11:53   ` [PATCH net-next 0/8] disable neigh update for tunnels during pmtu update Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 3/8] gtp: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-19 17:47       ` Guillaume Nault
2019-12-20  2:36         ` Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 6/8] vti: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 7/8] sit: " Hangbin Liu
2019-12-18 11:53     ` [PATCH net-next 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-19 17:49       ` Guillaume Nault
2019-12-18 12:01     ` [PATCH net-next 0/8] disable neigh update for tunnels during " Hangbin Liu
2019-12-19 17:57       ` Guillaume Nault
2019-12-20  2:48         ` Hangbin Liu
2019-12-19 17:53     ` Guillaume Nault
2019-12-20  3:25     ` [PATCHv4 net " Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 3/8] gtp: " Hangbin Liu
2019-12-21 18:35         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-21 18:43         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 6/8] vti: " Hangbin Liu
2019-12-21 18:30         ` Guillaume Nault
2019-12-20  3:25       ` [PATCHv4 net 7/8] sit: " Hangbin Liu
2019-12-20  3:25       ` [PATCHv4 net 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-21 18:38         ` Guillaume Nault
2019-12-20 16:14       ` [PATCHv4 net 0/8] disable neigh update for tunnels during " David Ahern
2019-12-22  2:51       ` [PATCHv5 " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 1/8] net: add bool confirm_neigh parameter for dst_ops.update_pmtu Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 2/8] ip6_gre: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 3/8] gtp: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 4/8] net/dst: add new function skb_dst_update_pmtu_no_confirm Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 5/8] tunnel: do not confirm neighbor when do pmtu update Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 6/8] vti: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 7/8] sit: " Hangbin Liu
2019-12-22  2:51         ` [PATCHv5 net 8/8] net/dst: do not confirm neighbor for vxlan and geneve " Hangbin Liu
2019-12-22 22:10         ` [PATCHv5 net 0/8] disable neigh update for tunnels during " Guillaume Nault
2019-12-25  6:30         ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191210170000.GA1132@linux.home \
    --to=gnault@redhat.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=edumazet@google.com \
    --cc=ja@ssi.bg \
    --cc=liuhangbin@gmail.com \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.