linux-next: origin tree build failure

linux-next: origin tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

Hi Roland,

Today's linux-next build (powerpc ppc64_defconfig) failed like this:

ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!

Caused by commit 38617c64bf9a10bf20e41d95b69bb81e8560fe9d ("RDMA/addr:
Add support for translating IPv6 addresses").  This requires a dependency
on IPV6.

I have reverted that commit for today (which fixes the build, but may not
be the correct solution).
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/


[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: linux-next: origin tree build failure

[Roland Dreier]

 > Today's linux-next build (powerpc ppc64_defconfig) failed like this:
 > 
 > ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
 > ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!
 > 
 > Caused by commit 38617c64bf9a10bf20e41d95b69bb81e8560fe9d ("RDMA/addr:
 > Add support for translating IPv6 addresses").  This requires a dependency
 > on IPV6.

Thanks, I'll get a fix queued up.  Sorry about that.

 - R.

Re: linux-next: origin tree build failure

[Roland Dreier]

 > ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
 > ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!
 > 
 > Caused by commit 38617c64bf9a10bf20e41d95b69bb81e8560fe9d ("RDMA/addr:
 > Add support for translating IPv6 addresses").  This requires a dependency
 > on IPV6.

So how do we want to fix this?  (This question is mostly directed to the
IB guys)  One possibility is to make all this depend on IPV6 in Kconfig,
but I think we want the RDMA CM to be buildable/usable even if IPv6
isn't enabled.  A better option is to just put all the IPv6 related
stuff into

#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

and add a Kconfig dependency on (IPV6 || IPV6=n) as we did for IPoIB.

But then this leads to the behavior that loading the RDMA CM will cause
the ipv6 module to be loaded if IPV6=m in the kernel config, even if the
administrator doesn't want to enable IPv6, just as with IPoIB today.
And people already complain about that.

Anyone see a better solution (which we could use for IPoIB even)?

 - R.

[ofa-general] Re: linux-next: origin tree build failure

[Aleksey Senin]

I'm going to change it by adding 

#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

exactly as it solved in ipoib module and as Roland says.

On Mon, 2008-12-29 at 11:43 +1100, Stephen Rothwell wrote:
> Hi Roland,
> 
> Today's linux-next build (powerpc ppc64_defconfig) failed like this:
> 
> ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
> ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!
> 
> Caused by commit 38617c64bf9a10bf20e41d95b69bb81e8560fe9d ("RDMA/addr:
> Add support for translating IPv6 addresses").  This requires a dependency
> on IPV6.
> 
> I have reverted that commit for today (which fixes the build, but may not
> be the correct solution).

Re: linux-next: origin tree build failure

[Aleksey Senin]

After another investigation of this problem, I think that proposed
solution is #ifdef as good for a first stage. IPv6 support is mandatory
when we are talking about running linux in some organization. But, of
course, the way how it implemented in IB stack should be changed. So on
the second stage, I'd like drop out these "defines" and at the time of
module initialization obtain addresses of IPv6 functions and in the case
if they are present at the runtime, call them. It should be nice
solution for RMDA_CM and IPoIB modules.
On Sun, 2008-12-28 at 19:44 -0800, Roland Dreier wrote:
> > ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
>  > ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!
>  > 
>  > Caused by commit 38617c64bf9a10bf20e41d95b69bb81e8560fe9d ("RDMA/addr:
>  > Add support for translating IPv6 addresses").  This requires a dependency
>  > on IPV6.
> 
> So how do we want to fix this?  (This question is mostly directed to the
> IB guys)  One possibility is to make all this depend on IPV6 in Kconfig,
> but I think we want the RDMA CM to be buildable/usable even if IPv6
> isn't enabled.  A better option is to just put all the IPv6 related
> stuff into
> 
> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> 
> and add a Kconfig dependency on (IPV6 || IPV6=n) as we did for IPoIB.
> 
> But then this leads to the behavior that loading the RDMA CM will cause
> the ipv6 module to be loaded if IPV6=m in the kernel config, even if the
> administrator doesn't want to enable IPv6, just as with IPoIB today.
> And people already complain about that.
> 
> Anyone see a better solution (which we could use for IPoIB even)?
> 
>  - R.

[ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 > After another investigation of this problem, I think that proposed
 > solution is #ifdef as good for a first stage. IPv6 support is mandatory
 > when we are talking about running linux in some organization. But, of
 > course, the way how it implemented in IB stack should be changed. So on
 > the second stage, I'd like drop out these "defines" and at the time of
 > module initialization obtain addresses of IPv6 functions and in the case
 > if they are present at the runtime, call them. It should be nice
 > solution for RMDA_CM and IPoIB modules.

I don't think this second stage sounds like a good idea.  Suppose
someone loads the RDMA CM first, so it doesn't find the ipv6 functions,
and then later loads and configures ipv6.  You'll end up in a situation
where trying to make an IPv6 connection fails spuriously.  (And just the
ugliness of looking up function pointers isn't very nice either)

 - R.

[ofa-general] Re: linux-next: origin tree build failure

[Aleksey Senin]

I thought about this.It can be solved by loading ipv6 module before
RDMA_CM by specifying modules dependencies in modprobe.conf file. At
least this solution helps in the the case when administrator want IB,
but not IPv6.
On Mon, 2008-12-29 at 08:13 -0800, Roland Dreier wrote:
> > After another investigation of this problem, I think that proposed
>  > solution is #ifdef as good for a first stage. IPv6 support is mandatory
>  > when we are talking about running linux in some organization. But, of
>  > course, the way how it implemented in IB stack should be changed. So on
>  > the second stage, I'd like drop out these "defines" and at the time of
>  > module initialization obtain addresses of IPv6 functions and in the case
>  > if they are present at the runtime, call them. It should be nice
>  > solution for RMDA_CM and IPoIB modules.
> 
> I don't think this second stage sounds like a good idea.  Suppose
> someone loads the RDMA CM first, so it doesn't find the ipv6 functions,
> and then later loads and configures ipv6.  You'll end up in a situation
> where trying to make an IPv6 connection fails spuriously.  (And just the
> ugliness of looking up function pointers isn't very nice either)
> 
>  - R.

[ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

Something like the following maybe?  (This turns off the RDMA CM if
INFINIBAND=y and IPV6=m -- another possibility would be to just turn off
RDMA CM IPv6 support in the case that IB is build-in but IPv6 is
modular, but that seems like a worse idea overall)

diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index a5dc78a..538a0ba 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -36,7 +36,7 @@ config INFINIBAND_USER_MEM
 
 config INFINIBAND_ADDR_TRANS
 	bool
-	depends on INET
+	depends on INET && !(INFINIBAND = y && IPV6 = m)
 	default y
 
 source "drivers/infiniband/hw/mthca/Kconfig"
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index d98b05b..ec7abb5 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -128,6 +128,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 		ret = rdma_copy_addr(dev_addr, dev, NULL);
 		dev_put(dev);
 		break;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		for_each_netdev(&init_net, dev) {
 			if (ipv6_chk_addr(&init_net,
@@ -138,6 +139,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 			}
 		}
 		break;
+#endif
 	default:
 		break;
 	}
@@ -179,10 +181,11 @@ static void addr_send_arp(struct sockaddr *dst_in)
 {
 	struct rtable *rt;
 	struct flowi fl;
-	struct dst_entry *dst;
 
 	memset(&fl, 0, sizeof fl);
-	if (dst_in->sa_family == AF_INET)  {
+
+	switch (dst_in->sa_family) {
+	case AF_INET:
 		fl.nl_u.ip4_u.daddr =
 			((struct sockaddr_in *) dst_in)->sin_addr.s_addr;
 
@@ -191,8 +194,13 @@ static void addr_send_arp(struct sockaddr *dst_in)
 
 		neigh_event_send(rt->u.dst.neighbour, NULL);
 		ip_rt_put(rt);
+		break;
+
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	case AF_INET6:
+	{
+		struct dst_entry *dst;
 
-	} else {
 		fl.nl_u.ip6_u.daddr =
 			((struct sockaddr_in6 *) dst_in)->sin6_addr;
 
@@ -202,6 +210,9 @@ static void addr_send_arp(struct sockaddr *dst_in)
 
 		neigh_event_send(dst->neighbour, NULL);
 		dst_release(dst);
+		break;
+	}
+#endif
 	}
 }
 
@@ -254,6 +265,7 @@ out:
 	return ret;
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 			       struct sockaddr_in6 *dst_in,
 			       struct rdma_dev_addr *addr)
@@ -282,6 +294,14 @@ static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
 	dst_release(dst);
 	return ret;
 }
+#else
+static int addr6_resolve_remote(struct sockaddr_in6 *src_in,
+			       struct sockaddr_in6 *dst_in,
+			       struct rdma_dev_addr *addr)
+{
+	return -EADDRNOTAVAIL;
+}
+#endif
 
 static int addr_resolve_remote(struct sockaddr *src_in,
 				struct sockaddr *dst_in,
@@ -340,7 +360,9 @@ static int addr_resolve_local(struct sockaddr *src_in,
 	struct net_device *dev;
 	int ret;
 
-	if (dst_in->sa_family == AF_INET) {
+	switch (dst_in->sa_family) {
+	case AF_INET:
+	{
 		__be32 src_ip = ((struct sockaddr_in *) src_in)->sin_addr.s_addr;
 		__be32 dst_ip = ((struct sockaddr_in *) dst_in)->sin_addr.s_addr;
 
@@ -362,7 +384,12 @@ static int addr_resolve_local(struct sockaddr *src_in,
 				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
 		}
 		dev_put(dev);
-	} else {
+		break;
+	}
+
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
+	case AF_INET6:
+	{
 		struct in6_addr *a;
 
 		for_each_netdev(&init_net, dev)
@@ -390,6 +417,13 @@ static int addr_resolve_local(struct sockaddr *src_in,
 			if (!ret)
 				memcpy(addr->dst_dev_addr, dev->dev_addr, MAX_ADDR_LEN);
 		}
+		break;
+	}
+#endif
+
+	default:
+		ret = -EADDRNOTAVAIL;
+		break;
 	}
 
 	return ret;

[ofa-general] Re: linux-next: origin tree build failure

[Linus Torvalds]

On Mon, 29 Dec 2008, Roland Dreier wrote:
>
> Something like the following maybe?  (This turns off the RDMA CM if
> INFINIBAND=y and IPV6=m -- another possibility would be to just turn off
> RDMA CM IPv6 support in the case that IB is build-in but IPv6 is
> modular, but that seems like a worse idea overall)
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index a5dc78a..538a0ba 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -36,7 +36,7 @@ config INFINIBAND_USER_MEM
>  
>  config INFINIBAND_ADDR_TRANS
>  	bool
> -	depends on INET
> +	depends on INET && !(INFINIBAND = y && IPV6 = m)
>  	default y

I'd suggest

  config IF_IPV6
	bool
	depends on INET
	depends on !(INFINIBAND = y && IPV6 = m)
	default y

and then you can use it not only for the above:

  config INFINIBAND_ADDR_TRANS
	bool
	depends on IF_IPV6
	default y

but also use it in the source code as a more readable version:

> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> index d98b05b..ec7abb5 100644
> --- a/drivers/infiniband/core/addr.c> +++ b/drivers/infiniband/core/addr.c
> @@ -128,6 +128,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
>  		ret = rdma_copy_addr(dev_addr, dev, NULL);
>  		dev_put(dev);
>  		break;
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

ie use

	#ifdef CONFIG_IF_IPV6

here instead.

		Linus

[ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 > I'd suggest
 > 
 >   config IF_IPV6
 > 	bool
 > 	depends on INET
 > 	depends on !(INFINIBAND = y && IPV6 = m)
 > 	default y

Makes sense, will do.  How about calling it INFINIBAND_USE_IPV6 or
something like that, though?  (Since it's under the INFINIBAND config
stuff and exists to forbid INFINIBAND=y && IPV6=m trying to use IPv6).
But see below:

 > but also use it in the source code as a more readable version:
 > 
 > > diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
 > > index d98b05b..ec7abb5 100644
 > > --- a/drivers/infiniband/core/addr.c> +++ b/drivers/infiniband/core/addr.c
 > > @@ -128,6 +128,7 @@ int rdma_translate_ip(struct sockaddr *addr, struct rdma_dev_addr *dev_addr)
 > >  		ret = rdma_copy_addr(dev_addr, dev, NULL);
 > >  		dev_put(dev);
 > >  		break;
 > > +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 > 
 > ie use
 > 
 > 	#ifdef CONFIG_IF_IPV6

this doesn't make sense, does it?  Your CONFIG_IF_IPV6 will be set in
the case IPV6=n too I think.  (Which is the whole point... we want to
build this code, just without IPv6 support, if IPv6 is turned off
completely)

 - R.

Re: [ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 >  > I'd suggest
 >  > 
 >  >   config IF_IPV6
 >  > 	bool
 >  > 	depends on INET
 >  > 	depends on !(INFINIBAND = y && IPV6 = m)
 >  > 	default y
 > 
 > Makes sense, will do.  How about calling it INFINIBAND_USE_IPV6 or
 > something like that, though?  (Since it's under the INFINIBAND config
 > stuff and exists to forbid INFINIBAND=y && IPV6=m trying to use IPv6).

Actually, thinking about this for 30 more seconds, I'm not sure how
another config symbol helps at all.  I do like splitting dependencies
onto multiple lines as a replacement for &&, so I have:

config INFINIBAND_ADDR_TRANS
	bool
	depends on INET
	depends on !(INFINIBAND = y && IPV6 = m)
	default y

right now.

Not sure if it's worth introducing another Kconfig symbol that depends
on IPV6 != n to avoid the

#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)

tests.  I note that there are tons of that construction all over the
tree, and the places without it look somewhat dubious (eg
net/ipv4/ip_gre.c looks as if it will do the wrong thing if IPV6=m).
Maybe adding CONFIG_IPV6_ENABLED or something and cleaning up the whole
tree would be a good janitorial project?

 - R.

[ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 > ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
 > ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!

OK, I pushed out a change that should fix this, so you can drop the
revert.  Let me know if you see further issues.

 - R.

[ofa-general] Re: linux-next: origin tree build failure

[Stephen Rothwell]

[-- Attachment #1.1: Type: text/plain, Size: 622 bytes --]

Hi Roland,

On Mon, 29 Dec 2008 23:38:18 -0800 Roland Dreier <rdreier@cisco.com> wrote:
>
>  > ERROR: ".ipv6_chk_addr" [drivers/infiniband/core/ib_addr.ko] undefined!
>  > ERROR: ".ip6_route_output" [drivers/infiniband/core/ib_addr.ko] undefined!
> 
> OK, I pushed out a change that should fix this, so you can drop the
> revert.  Let me know if you see further issues.

I can drop the revert after your fix goes into Linus' tree (since that is
where the breakage is) ... looking forward to it, thanks.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 > I can drop the revert after your fix goes into Linus' tree (since that is
 > where the breakage is) ... looking forward to it, thanks.

Shouldn't it work if the fix is in my for-next branch, since you pull
that as part of the -next tree?  Or am I unclear on how -next works?

 - R.

Re: [ofa-general] Re: linux-next: origin tree build failure

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

Hi Roland,

On Tue, 30 Dec 2008 07:41:36 -0800 Roland Dreier <rdreier@cisco.com> wrote:
>
>  > I can drop the revert after your fix goes into Linus' tree (since that is
>  > where the breakage is) ... looking forward to it, thanks.
> 
> Shouldn't it work if the fix is in my for-next branch, since you pull
> that as part of the -next tree?  Or am I unclear on how -next works?

I also build the tree between merging most of the trees (including after
fetching Linus' latest tree).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [ofa-general] Re: linux-next: origin tree build failure

[Stephen Rothwell]

[-- Attachment #1.1: Type: text/plain, Size: 856 bytes --]

Hi Roland,

On Wed, 31 Dec 2008 02:46:54 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Tue, 30 Dec 2008 07:41:36 -0800 Roland Dreier <rdreier@cisco.com> wrote:
> >
> >  > I can drop the revert after your fix goes into Linus' tree (since that is
> >  > where the breakage is) ... looking forward to it, thanks.
> > 
> > Shouldn't it work if the fix is in my for-next branch, since you pull
> > that as part of the -next tree?  Or am I unclear on how -next works?
> 
> I also build the tree between merging most of the trees (including after
> fetching Linus' latest tree).

So instead of the revert, I will cherry-pick your fix commit just after
Linus' tree today on the assumption that you will send it to him ASAP.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ofa-general] Re: linux-next: origin tree build failure

[Roland Dreier]

 > So instead of the revert, I will cherry-pick your fix commit just after
 > Linus' tree today on the assumption that you will send it to him ASAP.

Yes, I will batch up a few other things and send a pull request today.

 - R.

Re: [ofa-general] Re: linux-next: origin tree build failure

[Stephen Rothwell]

[-- Attachment #1.1: Type: text/plain, Size: 450 bytes --]

Hi Roland,

On Tue, 30 Dec 2008 14:56:05 -0800 Roland Dreier <rdreier@cisco.com> wrote:
>
>  > So instead of the revert, I will cherry-pick your fix commit just after
>  > Linus' tree today on the assumption that you will send it to him ASAP.
> 
> Yes, I will batch up a few other things and send a pull request today.

Excellent!

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]