netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
@ 2013-08-28 18:24 Tim Gardner
  2013-08-28 18:51 ` Joe Perches
  2013-08-28 23:36 ` Eric W. Biederman
  0 siblings, 2 replies; 9+ messages in thread
From: Tim Gardner @ 2013-08-28 18:24 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: Tim Gardner, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, Eric W. Biederman, Gao feng,
	Joe Perches

Drop a couple of ifdef/endif pairs by moving the ifdef
surrounding neigh_app_ns() to the interior of neigh_app_ns().

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---

This is an admittedly trivial change. I stumbled on it while trying to figure
out why Ubuntu doesn't have CONFIG_ARPD enabled.

 net/core/neighbour.c |    4 ++--
 net/ipv4/arp.c       |    2 --
 net/ipv6/ndisc.c     |    2 --
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 60533db..049dd9e 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2759,13 +2759,13 @@ errout:
 		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
 }
 
-#ifdef CONFIG_ARPD
 void neigh_app_ns(struct neighbour *n)
 {
+#ifdef CONFIG_ARPD
 	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
+#endif /* CONFIG_ARPD */
 }
 EXPORT_SYMBOL(neigh_app_ns);
-#endif /* CONFIG_ARPD */
 
 #ifdef CONFIG_SYSCTL
 static int zero;
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 4429b01..7808093 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	} else {
 		probes -= neigh->parms->app_probes;
 		if (probes < 0) {
-#ifdef CONFIG_ARPD
 			neigh_app_ns(neigh);
-#endif
 			return;
 		}
 	}
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 04d31c2..d5693ad 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
 		}
 		ndisc_send_ns(dev, neigh, target, target, saddr);
 	} else if ((probes -= neigh->parms->app_probes) < 0) {
-#ifdef CONFIG_ARPD
 		neigh_app_ns(neigh);
-#endif
 	} else {
 		addrconf_addr_solict_mult(target, &mcaddr);
 		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
  2013-08-28 18:24 [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns() Tim Gardner
@ 2013-08-28 18:51 ` Joe Perches
  2013-08-28 19:09   ` Tim Gardner
  2013-08-28 23:36 ` Eric W. Biederman
  1 sibling, 1 reply; 9+ messages in thread
From: Joe Perches @ 2013-08-28 18:51 UTC (permalink / raw)
  To: Tim Gardner
  Cc: netdev, linux-kernel, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy,
	Eric W. Biederman, Gao feng

On Wed, 2013-08-28 at 12:24 -0600, Tim Gardner wrote:
> Drop a couple of ifdef/endif pairs by moving the ifdef
> surrounding neigh_app_ns() to the interior of neigh_app_ns().
[]
> This is an admittedly trivial change. I stumbled on it while trying to figure
> out why Ubuntu doesn't have CONFIG_ARPD enabled.

I'd be more inclined to make neigh_app_ns static inline
in the .h file and remove the EXPORT_SYMBOL

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 60533db..049dd9e 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2759,13 +2759,13 @@ errout:
>  		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>  }
>  
> -#ifdef CONFIG_ARPD
>  void neigh_app_ns(struct neighbour *n)
>  {
> +#ifdef CONFIG_ARPD
>  	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
> +#endif /* CONFIG_ARPD */
>  }
>  EXPORT_SYMBOL(neigh_app_ns);
> -#endif /* CONFIG_ARPD */
>  
>  #ifdef CONFIG_SYSCTL
>  static int zero;
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4429b01..7808093 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	} else {
>  		probes -= neigh->parms->app_probes;
>  		if (probes < 0) {
> -#ifdef CONFIG_ARPD
>  			neigh_app_ns(neigh);
> -#endif
>  			return;
>  		}
>  	}
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 04d31c2..d5693ad 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  		}
>  		ndisc_send_ns(dev, neigh, target, target, saddr);
>  	} else if ((probes -= neigh->parms->app_probes) < 0) {
> -#ifdef CONFIG_ARPD
>  		neigh_app_ns(neigh);
> -#endif
>  	} else {
>  		addrconf_addr_solict_mult(target, &mcaddr);
>  		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
  2013-08-28 18:51 ` Joe Perches
@ 2013-08-28 19:09   ` Tim Gardner
  2013-08-29  1:26     ` Joe Perches
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Gardner @ 2013-08-28 19:09 UTC (permalink / raw)
  To: Joe Perches
  Cc: netdev, linux-kernel, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy,
	Eric W. Biederman, Gao feng

On 08/28/2013 12:51 PM, Joe Perches wrote:
> On Wed, 2013-08-28 at 12:24 -0600, Tim Gardner wrote:
>> Drop a couple of ifdef/endif pairs by moving the ifdef
>> surrounding neigh_app_ns() to the interior of neigh_app_ns().
> []
>> This is an admittedly trivial change. I stumbled on it while trying to figure
>> out why Ubuntu doesn't have CONFIG_ARPD enabled.
> 
> I'd be more inclined to make neigh_app_ns static inline
> in the .h file and remove the EXPORT_SYMBOL
> 

I thought about that as well, but then you'd have to extern
__neigh_notify(), which is currently a static function and large enough
to not really be suitable for inlining. Seems like unnecessary churn to me.

rtg
-- 
Tim Gardner tim.gardner@canonical.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
  2013-08-28 18:24 [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns() Tim Gardner
  2013-08-28 18:51 ` Joe Perches
@ 2013-08-28 23:36 ` Eric W. Biederman
  2013-08-29 12:38   ` [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD Tim Gardner
  1 sibling, 1 reply; 9+ messages in thread
From: Eric W. Biederman @ 2013-08-28 23:36 UTC (permalink / raw)
  To: Tim Gardner
  Cc: netdev, linux-kernel, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Gao feng,
	Joe Perches

Tim Gardner <tim.gardner@canonical.com> writes:

> Drop a couple of ifdef/endif pairs by moving the ifdef
> surrounding neigh_app_ns() to the interior of neigh_app_ns().

Can we please just remove the CONFIG_ARPD option entirely.

There really is no savings to keeping this option configurable and the
option only removes a netlink notification.

> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Gao feng <gaofeng@cn.fujitsu.com>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>
> This is an admittedly trivial change. I stumbled on it while trying to figure
> out why Ubuntu doesn't have CONFIG_ARPD enabled.
>
>  net/core/neighbour.c |    4 ++--
>  net/ipv4/arp.c       |    2 --
>  net/ipv6/ndisc.c     |    2 --
>  3 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 60533db..049dd9e 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2759,13 +2759,13 @@ errout:
>  		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>  }
>  
> -#ifdef CONFIG_ARPD
>  void neigh_app_ns(struct neighbour *n)
>  {
> +#ifdef CONFIG_ARPD
>  	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
> +#endif /* CONFIG_ARPD */
>  }
>  EXPORT_SYMBOL(neigh_app_ns);
> -#endif /* CONFIG_ARPD */
>  
>  #ifdef CONFIG_SYSCTL
>  static int zero;
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4429b01..7808093 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	} else {
>  		probes -= neigh->parms->app_probes;
>  		if (probes < 0) {
> -#ifdef CONFIG_ARPD
>  			neigh_app_ns(neigh);
> -#endif
>  			return;
>  		}
>  	}
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 04d31c2..d5693ad 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  		}
>  		ndisc_send_ns(dev, neigh, target, target, saddr);
>  	} else if ((probes -= neigh->parms->app_probes) < 0) {
> -#ifdef CONFIG_ARPD
>  		neigh_app_ns(neigh);
> -#endif
>  	} else {
>  		addrconf_addr_solict_mult(target, &mcaddr);
>  		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
  2013-08-28 19:09   ` Tim Gardner
@ 2013-08-29  1:26     ` Joe Perches
  2013-08-29  1:32       ` Eric W. Biederman
  0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2013-08-29  1:26 UTC (permalink / raw)
  To: Tim Gardner
  Cc: netdev, linux-kernel, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy,
	Eric W. Biederman, Gao feng

On Wed, 2013-08-28 at 13:09 -0600, Tim Gardner wrote:
> On 08/28/2013 12:51 PM, Joe Perches wrote:
> > On Wed, 2013-08-28 at 12:24 -0600, Tim Gardner wrote:
> >> Drop a couple of ifdef/endif pairs by moving the ifdef
> >> surrounding neigh_app_ns() to the interior of neigh_app_ns().
> > []
> >> This is an admittedly trivial change. I stumbled on it while trying to figure
> >> out why Ubuntu doesn't have CONFIG_ARPD enabled.

> > I'd be more inclined to make neigh_app_ns static inline
> > in the .h file and remove the EXPORT_SYMBOL

> I thought about that as well, but then you'd have to extern
> __neigh_notify(), which is currently a static function and large enough
> to not really be suitable for inlining. Seems like unnecessary churn to me.

Hi Tim.

As is, this makes the call to neight_app_ns
impossible to optimize away.

Perhaps this:

(this does add a possibly unused neigh_notify as a global symbol)

Rename __neigh_notify to neigh_notify and make public
Add static inline neigh_app_ns
Remove #ifdefs around use of neigh_app_ns

---

Compile tested only

 include/net/neighbour.h | 10 +++++++++-
 net/core/neighbour.c    | 15 +++------------
 net/ipv4/arp.c          |  2 --
 net/ipv6/ndisc.c        |  2 --
 4 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 536501a..a08b0a7 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -249,7 +249,15 @@ static inline struct net *pneigh_net(const struct pneigh_entry *pneigh)
 	return read_pnet(&pneigh->net);
 }
 
-void neigh_app_ns(struct neighbour *n);
+void neigh_notify(struct neighbour *n, int type, int flags);
+
+static inline void neigh_app_ns(struct neighbour *n)
+{
+#ifdef CONFIG_ARPD
+	neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
+#endif
+}
+
 void neigh_for_each(struct neigh_table *tbl,
 		    void (*cb)(struct neighbour *, void *), void *cookie);
 void __neigh_for_each_release(struct neigh_table *tbl,
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 60533db..6ec5f86 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -50,7 +50,6 @@ do {						\
 #define PNEIGH_HASHMASK		0xF
 
 static void neigh_timer_handler(unsigned long arg);
-static void __neigh_notify(struct neighbour *n, int type, int flags);
 static void neigh_update_notify(struct neighbour *neigh);
 static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev);
 
@@ -103,7 +102,7 @@ static void neigh_cleanup_and_release(struct neighbour *neigh)
 	if (neigh->parms->neigh_cleanup)
 		neigh->parms->neigh_cleanup(neigh);
 
-	__neigh_notify(neigh, RTM_DELNEIGH, 0);
+	neigh_notify(neigh, RTM_DELNEIGH, 0);
 	neigh_release(neigh);
 }
 
@@ -2215,7 +2214,7 @@ nla_put_failure:
 static void neigh_update_notify(struct neighbour *neigh)
 {
 	call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, neigh);
-	__neigh_notify(neigh, RTM_NEWNEIGH, 0);
+	neigh_notify(neigh, RTM_NEWNEIGH, 0);
 }
 
 static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
@@ -2735,7 +2734,7 @@ static inline size_t neigh_nlmsg_size(void)
 	       + nla_total_size(4); /* NDA_PROBES */
 }
 
-static void __neigh_notify(struct neighbour *n, int type, int flags)
+void neigh_notify(struct neighbour *n, int type, int flags)
 {
 	struct net *net = dev_net(n->dev);
 	struct sk_buff *skb;
@@ -2759,14 +2758,6 @@ errout:
 		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
 }
 
-#ifdef CONFIG_ARPD
-void neigh_app_ns(struct neighbour *n)
-{
-	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
-}
-EXPORT_SYMBOL(neigh_app_ns);
-#endif /* CONFIG_ARPD */
-
 #ifdef CONFIG_SYSCTL
 static int zero;
 static int int_max = INT_MAX;
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 4429b01..7808093 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	} else {
 		probes -= neigh->parms->app_probes;
 		if (probes < 0) {
-#ifdef CONFIG_ARPD
 			neigh_app_ns(neigh);
-#endif
 			return;
 		}
 	}
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 04d31c2..d5693ad 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
 		}
 		ndisc_send_ns(dev, neigh, target, target, saddr);
 	} else if ((probes -= neigh->parms->app_probes) < 0) {
-#ifdef CONFIG_ARPD
 		neigh_app_ns(neigh);
-#endif
 	} else {
 		addrconf_addr_solict_mult(target, &mcaddr);
 		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns()
  2013-08-29  1:26     ` Joe Perches
@ 2013-08-29  1:32       ` Eric W. Biederman
  0 siblings, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2013-08-29  1:32 UTC (permalink / raw)
  To: Joe Perches
  Cc: Tim Gardner, netdev, linux-kernel, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, Gao feng

Joe Perches <joe@perches.com> writes:

> On Wed, 2013-08-28 at 13:09 -0600, Tim Gardner wrote:
>> On 08/28/2013 12:51 PM, Joe Perches wrote:
>> > On Wed, 2013-08-28 at 12:24 -0600, Tim Gardner wrote:
>> >> Drop a couple of ifdef/endif pairs by moving the ifdef
>> >> surrounding neigh_app_ns() to the interior of neigh_app_ns().
>> > []
>> >> This is an admittedly trivial change. I stumbled on it while trying to figure
>> >> out why Ubuntu doesn't have CONFIG_ARPD enabled.
>
>> > I'd be more inclined to make neigh_app_ns static inline
>> > in the .h file and remove the EXPORT_SYMBOL
>
>> I thought about that as well, but then you'd have to extern
>> __neigh_notify(), which is currently a static function and large enough
>> to not really be suitable for inlining. Seems like unnecessary churn to me.
>
> Hi Tim.
>
> As is, this makes the call to neight_app_ns
> impossible to optimize away.
>
> Perhaps this:
>
> (this does add a possibly unused neigh_notify as a global symbol)
>
> Rename __neigh_notify to neigh_notify and make public
> Add static inline neigh_app_ns
> Remove #ifdefs around use of neigh_app_ns

Again.  Why not just remove CONFIG_ARPD entirely?  A config option to
compile out a single line of code seems like a real waste.

Eric

> ---
>
> Compile tested only
>
>  include/net/neighbour.h | 10 +++++++++-
>  net/core/neighbour.c    | 15 +++------------
>  net/ipv4/arp.c          |  2 --
>  net/ipv6/ndisc.c        |  2 --
>  4 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 536501a..a08b0a7 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -249,7 +249,15 @@ static inline struct net *pneigh_net(const struct pneigh_entry *pneigh)
>  	return read_pnet(&pneigh->net);
>  }
>  
> -void neigh_app_ns(struct neighbour *n);
> +void neigh_notify(struct neighbour *n, int type, int flags);
> +
> +static inline void neigh_app_ns(struct neighbour *n)
> +{
> +#ifdef CONFIG_ARPD
> +	neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
> +#endif
> +}
> +
>  void neigh_for_each(struct neigh_table *tbl,
>  		    void (*cb)(struct neighbour *, void *), void *cookie);
>  void __neigh_for_each_release(struct neigh_table *tbl,
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 60533db..6ec5f86 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -50,7 +50,6 @@ do {						\
>  #define PNEIGH_HASHMASK		0xF
>  
>  static void neigh_timer_handler(unsigned long arg);
> -static void __neigh_notify(struct neighbour *n, int type, int flags);
>  static void neigh_update_notify(struct neighbour *neigh);
>  static int pneigh_ifdown(struct neigh_table *tbl, struct net_device *dev);
>  
> @@ -103,7 +102,7 @@ static void neigh_cleanup_and_release(struct neighbour *neigh)
>  	if (neigh->parms->neigh_cleanup)
>  		neigh->parms->neigh_cleanup(neigh);
>  
> -	__neigh_notify(neigh, RTM_DELNEIGH, 0);
> +	neigh_notify(neigh, RTM_DELNEIGH, 0);
>  	neigh_release(neigh);
>  }
>  
> @@ -2215,7 +2214,7 @@ nla_put_failure:
>  static void neigh_update_notify(struct neighbour *neigh)
>  {
>  	call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, neigh);
> -	__neigh_notify(neigh, RTM_NEWNEIGH, 0);
> +	neigh_notify(neigh, RTM_NEWNEIGH, 0);
>  }
>  
>  static int neigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
> @@ -2735,7 +2734,7 @@ static inline size_t neigh_nlmsg_size(void)
>  	       + nla_total_size(4); /* NDA_PROBES */
>  }
>  
> -static void __neigh_notify(struct neighbour *n, int type, int flags)
> +void neigh_notify(struct neighbour *n, int type, int flags)
>  {
>  	struct net *net = dev_net(n->dev);
>  	struct sk_buff *skb;
> @@ -2759,14 +2758,6 @@ errout:
>  		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>  }
>  
> -#ifdef CONFIG_ARPD
> -void neigh_app_ns(struct neighbour *n)
> -{
> -	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
> -}
> -EXPORT_SYMBOL(neigh_app_ns);
> -#endif /* CONFIG_ARPD */
> -
>  #ifdef CONFIG_SYSCTL
>  static int zero;
>  static int int_max = INT_MAX;
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4429b01..7808093 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	} else {
>  		probes -= neigh->parms->app_probes;
>  		if (probes < 0) {
> -#ifdef CONFIG_ARPD
>  			neigh_app_ns(neigh);
> -#endif
>  			return;
>  		}
>  	}
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 04d31c2..d5693ad 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  		}
>  		ndisc_send_ns(dev, neigh, target, target, saddr);
>  	} else if ((probes -= neigh->parms->app_probes) < 0) {
> -#ifdef CONFIG_ARPD
>  		neigh_app_ns(neigh);
> -#endif
>  	} else {
>  		addrconf_addr_solict_mult(target, &mcaddr);
>  		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD
  2013-08-28 23:36 ` Eric W. Biederman
@ 2013-08-29 12:38   ` Tim Gardner
  2013-08-29 23:39     ` Eric W. Biederman
  2013-09-04  1:42     ` David Miller
  0 siblings, 2 replies; 9+ messages in thread
From: Tim Gardner @ 2013-08-29 12:38 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: Tim Gardner, David S. Miller, Alexey Kuznetsov, James Morris,
	Hideaki YOSHIFUJI, Patrick McHardy, Eric W. Biederman, Gao feng,
	Joe Perches, Veaceslav Falico

This config option is superfluous in that it only guards a call
to neigh_app_ns(). Enabling CONFIG_ARPD by default has no
change in behavior. There will now be call to __neigh_notify()
for each ARP resolution, which has no impact unless there is a
user space daemon waiting to receive the notification, i.e.,
the case for which CONFIG_ARPD was designed anyways.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Gao feng <gaofeng@cn.fujitsu.com>
Cc: Joe Perches <joe@perches.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---

Eric's suggestion to simply remove the config option makes sense
to me. If acceptable then I'll submit a patch series that also removes
CONFIG_ARPD from the various arch defconfigs.

 net/core/neighbour.c |    2 --
 net/ipv4/Kconfig     |   16 ----------------
 net/ipv4/arp.c       |    2 --
 net/ipv6/ndisc.c     |    2 --
 4 files changed, 22 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 60533db..6072610 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -2759,13 +2759,11 @@ errout:
 		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
 }
 
-#ifdef CONFIG_ARPD
 void neigh_app_ns(struct neighbour *n)
 {
 	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
 }
 EXPORT_SYMBOL(neigh_app_ns);
-#endif /* CONFIG_ARPD */
 
 #ifdef CONFIG_SYSCTL
 static int zero;
diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
index 37cf1a6..05c57f0 100644
--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -259,22 +259,6 @@ config IP_PIMSM_V2
 	  gated-5). This routing protocol is not used widely, so say N unless
 	  you want to play with it.
 
-config ARPD
-	bool "IP: ARP daemon support"
-	---help---
-	  The kernel maintains an internal cache which maps IP addresses to
-	  hardware addresses on the local network, so that Ethernet
-	  frames are sent to the proper address on the physical networking
-	  layer. Normally, kernel uses the ARP protocol to resolve these
-	  mappings.
-
-	  Saying Y here adds support to have an user space daemon to do this
-	  resolution instead. This is useful for implementing an alternate
-	  address resolution protocol (e.g. NHRP on mGRE tunnels) and also for
-	  testing purposes.
-
-	  If unsure, say N.
-
 config SYN_COOKIES
 	bool "IP: TCP syncookie support"
 	---help---
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 4429b01..7808093 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
 	} else {
 		probes -= neigh->parms->app_probes;
 		if (probes < 0) {
-#ifdef CONFIG_ARPD
 			neigh_app_ns(neigh);
-#endif
 			return;
 		}
 	}
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 04d31c2..d5693ad 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
 		}
 		ndisc_send_ns(dev, neigh, target, target, saddr);
 	} else if ((probes -= neigh->parms->app_probes) < 0) {
-#ifdef CONFIG_ARPD
 		neigh_app_ns(neigh);
-#endif
 	} else {
 		addrconf_addr_solict_mult(target, &mcaddr);
 		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD
  2013-08-29 12:38   ` [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD Tim Gardner
@ 2013-08-29 23:39     ` Eric W. Biederman
  2013-09-04  1:42     ` David Miller
  1 sibling, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2013-08-29 23:39 UTC (permalink / raw)
  To: Tim Gardner
  Cc: netdev, linux-kernel, David S. Miller, Alexey Kuznetsov,
	James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Gao feng,
	Joe Perches, Veaceslav Falico

Tim Gardner <tim.gardner@canonical.com> writes:

> This config option is superfluous in that it only guards a call
> to neigh_app_ns(). Enabling CONFIG_ARPD by default has no
> change in behavior. There will now be call to __neigh_notify()
> for each ARP resolution, which has no impact unless there is a
> user space daemon waiting to receive the notification, i.e.,
> the case for which CONFIG_ARPD was designed anyways.

This looks good to me, and much less magic to maintain.

Eric


> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Gao feng <gaofeng@cn.fujitsu.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>
> Eric's suggestion to simply remove the config option makes sense
> to me. If acceptable then I'll submit a patch series that also removes
> CONFIG_ARPD from the various arch defconfigs.
>
>  net/core/neighbour.c |    2 --
>  net/ipv4/Kconfig     |   16 ----------------
>  net/ipv4/arp.c       |    2 --
>  net/ipv6/ndisc.c     |    2 --
>  4 files changed, 22 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 60533db..6072610 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2759,13 +2759,11 @@ errout:
>  		rtnl_set_sk_err(net, RTNLGRP_NEIGH, err);
>  }
>  
> -#ifdef CONFIG_ARPD
>  void neigh_app_ns(struct neighbour *n)
>  {
>  	__neigh_notify(n, RTM_GETNEIGH, NLM_F_REQUEST);
>  }
>  EXPORT_SYMBOL(neigh_app_ns);
> -#endif /* CONFIG_ARPD */
>  
>  #ifdef CONFIG_SYSCTL
>  static int zero;
> diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
> index 37cf1a6..05c57f0 100644
> --- a/net/ipv4/Kconfig
> +++ b/net/ipv4/Kconfig
> @@ -259,22 +259,6 @@ config IP_PIMSM_V2
>  	  gated-5). This routing protocol is not used widely, so say N unless
>  	  you want to play with it.
>  
> -config ARPD
> -	bool "IP: ARP daemon support"
> -	---help---
> -	  The kernel maintains an internal cache which maps IP addresses to
> -	  hardware addresses on the local network, so that Ethernet
> -	  frames are sent to the proper address on the physical networking
> -	  layer. Normally, kernel uses the ARP protocol to resolve these
> -	  mappings.
> -
> -	  Saying Y here adds support to have an user space daemon to do this
> -	  resolution instead. This is useful for implementing an alternate
> -	  address resolution protocol (e.g. NHRP on mGRE tunnels) and also for
> -	  testing purposes.
> -
> -	  If unsure, say N.
> -
>  config SYN_COOKIES
>  	bool "IP: TCP syncookie support"
>  	---help---
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 4429b01..7808093 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -368,9 +368,7 @@ static void arp_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  	} else {
>  		probes -= neigh->parms->app_probes;
>  		if (probes < 0) {
> -#ifdef CONFIG_ARPD
>  			neigh_app_ns(neigh);
> -#endif
>  			return;
>  		}
>  	}
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 04d31c2..d5693ad 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -663,9 +663,7 @@ static void ndisc_solicit(struct neighbour *neigh, struct sk_buff *skb)
>  		}
>  		ndisc_send_ns(dev, neigh, target, target, saddr);
>  	} else if ((probes -= neigh->parms->app_probes) < 0) {
> -#ifdef CONFIG_ARPD
>  		neigh_app_ns(neigh);
> -#endif
>  	} else {
>  		addrconf_addr_solict_mult(target, &mcaddr);
>  		ndisc_send_ns(dev, NULL, target, &mcaddr, saddr);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD
  2013-08-29 12:38   ` [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD Tim Gardner
  2013-08-29 23:39     ` Eric W. Biederman
@ 2013-09-04  1:42     ` David Miller
  1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2013-09-04  1:42 UTC (permalink / raw)
  To: tim.gardner
  Cc: netdev, linux-kernel, kuznet, jmorris, yoshfuji, kaber, ebiederm,
	gaofeng, joe, vfalico

From: Tim Gardner <tim.gardner@canonical.com>
Date: Thu, 29 Aug 2013 06:38:47 -0600

> This config option is superfluous in that it only guards a call
> to neigh_app_ns(). Enabling CONFIG_ARPD by default has no
> change in behavior. There will now be call to __neigh_notify()
> for each ARP resolution, which has no impact unless there is a
> user space daemon waiting to receive the notification, i.e.,
> the case for which CONFIG_ARPD was designed anyways.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
 ...
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-09-04  1:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-28 18:24 [PATCH net-next 1/1] net: neighbour: Simplify ifdefs around neigh_app_ns() Tim Gardner
2013-08-28 18:51 ` Joe Perches
2013-08-28 19:09   ` Tim Gardner
2013-08-29  1:26     ` Joe Perches
2013-08-29  1:32       ` Eric W. Biederman
2013-08-28 23:36 ` Eric W. Biederman
2013-08-29 12:38   ` [PATCH net-next v2] net: neighbour: Remove CONFIG_ARPD Tim Gardner
2013-08-29 23:39     ` Eric W. Biederman
2013-09-04  1:42     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).