| Message ID | 20160310.130138.1302349043066531127.davem@davemloft.net |
|---|---|
| State | RFC |
| Headers | show |
On Thu, Mar 10, 2016 at 01:01:38PM -0500, David Miller wrote: > From: Cyrill Gorcunov <gorcunov@gmail.com> > Date: Thu, 10 Mar 2016 18:09:20 +0300 > > > On Thu, Mar 10, 2016 at 02:03:24PM +0300, Cyrill Gorcunov wrote: > >> On Thu, Mar 10, 2016 at 01:20:18PM +0300, Cyrill Gorcunov wrote: > >> > On Thu, Mar 10, 2016 at 12:16:29AM +0300, Cyrill Gorcunov wrote: > >> > > > >> > > Thanks for explanation, Dave! I'll continue on this task tomorrow > >> > > tryin to implement optimization you proposed. > >> > > >> > OK, here are the results for the preliminary patch with conntrack running > >> ... > >> > net/ipv4/devinet.c | 13 ++++++++++++- > >> > 1 file changed, 12 insertions(+), 1 deletion(-) > >> > > >> > Index: linux-ml.git/net/ipv4/devinet.c > >> > =================================================================== > >> > --- linux-ml.git.orig/net/ipv4/devinet.c > >> > +++ linux-ml.git/net/ipv4/devinet.c > >> > @@ -403,7 +403,18 @@ no_promotions: > >> > So that, this order is correct. > >> > */ > >> > >> This patch is wrong, so drop it please. I'll do another. > > > > Here I think is a better variant. The resulst are good > > enough -- 1 sec for cleanup. Does the patch look sane? > > I'm tempted to say that we should provide these notifier handlers with > the information they need, explicitly, to handle this case. > > Most intdev notifiers actually want to know the individual addresses > that get removed, one by one. That's handled by the existing > NETDEV_DOWN event and the ifa we pass to that. > > But some, like this netfilter masq case, would be satisfied with a > single event that tells them the whole inetdev instance is being torn > down. Which is the case we care about here. > > We currently don't use NETDEV_UNREGISTER for inetdev notifiers, so > maybe we could use that. > > And that is consistent with the core netdev notifier that triggers > this call chain in the first place. > > Roughly, something like this: I see. Dave, gimme some time to test but I'm sure it'll work. I don't have some strong opinion here, so your patch looks pretty fine to me. But maybe people from netdev camp have some other ideas. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 10, 2016 at 10:01 AM, David Miller <davem@davemloft.net> wrote: > I'm tempted to say that we should provide these notifier handlers with > the information they need, explicitly, to handle this case. > > Most intdev notifiers actually want to know the individual addresses > that get removed, one by one. That's handled by the existing > NETDEV_DOWN event and the ifa we pass to that. > > But some, like this netfilter masq case, would be satisfied with a > single event that tells them the whole inetdev instance is being torn > down. Which is the case we care about here. > > We currently don't use NETDEV_UNREGISTER for inetdev notifiers, so > maybe we could use that. > > And that is consistent with the core netdev notifier that triggers > this call chain in the first place. > > Roughly, something like this: > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index 8c3df2c..6eee5cb 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -292,6 +292,11 @@ static void inetdev_destroy(struct in_device *in_dev) > > in_dev->dead = 1; > > + if (in_dev->ifa_list) > + blocking_notifier_call_chain(&inetaddr_chain, > + NETDEV_UNREGISTER, > + in_dev->ifa_list); > + > ip_mc_destroy_dev(in_dev); Hmm, but inetdev_destroy() is only called when NETDEV_UNREGISTER is happening and masq already registers a netdev notifier... > > while ((ifa = in_dev->ifa_list) != NULL) { > diff --git a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c > index c6eb421..1bb8026 100644 > --- a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c > +++ b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c > @@ -111,6 +111,10 @@ static int masq_inet_event(struct notifier_block *this, > struct net_device *dev = ((struct in_ifaddr *)ptr)->ifa_dev->dev; > struct netdev_notifier_info info; > > + if (event != NETDEV_UNREGISTER) > + return NOTIFY_DONE; > + event = NETDEV_DOWN; > + > netdev_notifier_info_init(&info, dev); > return masq_device_event(this, event, &info); > } If masq really doesn't care about inetdev destroy or inetaddr removal, we should just remove its inetaddr notifier. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 8c3df2c..6eee5cb 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -292,6 +292,11 @@ static void inetdev_destroy(struct in_device *in_dev) in_dev->dead = 1; + if (in_dev->ifa_list) + blocking_notifier_call_chain(&inetaddr_chain, + NETDEV_UNREGISTER, + in_dev->ifa_list); + ip_mc_destroy_dev(in_dev); while ((ifa = in_dev->ifa_list) != NULL) { diff --git a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c index c6eb421..1bb8026 100644 --- a/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c +++ b/net/ipv4/netfilter/nf_nat_masquerade_ipv4.c @@ -111,6 +111,10 @@ static int masq_inet_event(struct notifier_block *this, struct net_device *dev = ((struct in_ifaddr *)ptr)->ifa_dev->dev; struct netdev_notifier_info info; + if (event != NETDEV_UNREGISTER) + return NOTIFY_DONE; + event = NETDEV_DOWN; + netdev_notifier_info_init(&info, dev); return masq_device_event(this, event, &info); }