linux-next: build failure after merge of the xarray tree

linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

After merging the xarray tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

In file included from include/linux/uio.h:12,
                 from include/linux/socket.h:8,
                 from include/rdma/rdma_cm.h:37,
                 from drivers/infiniband/core/restrack.c:6:
drivers/infiniband/core/restrack.c: In function 'rt_xa_alloc_cyclic':
include/linux/kernel.h:40:18: warning: passing argument 3 of '__xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
 #define U32_MAX  ((u32)~0U)
                  ^~~~~~~~~~
drivers/infiniband/core/restrack.c:26:27: note: in expansion of macro 'U32_MAX'
  err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
                           ^~~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/fs.h:15,
                 from include/linux/seq_file.h:11,
                 from arch/powerpc/include/asm/machdep.h:12,
                 from arch/powerpc/include/asm/archrandom.h:7,
                 from include/linux/random.h:166,
                 from include/linux/net.h:22,
                 from include/linux/skbuff.h:29,
                 from include/linux/if_arp.h:26,
                 from include/rdma/ib_addr.h:39,
                 from include/rdma/rdma_cm.h:39,
                 from drivers/infiniband/core/restrack.c:6:
include/linux/xarray.h:524:61: note: expected 'void *' but argument is of type 'unsigned int'
 int __must_check __xa_alloc(struct xarray *, u32 *id, void *entry,
                                                       ~~~~~~^~~~~
drivers/infiniband/core/restrack.c:26:36: error: incompatible type for argument 4 of '__xa_alloc'
  err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
                                    ^~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/fs.h:15,
                 from include/linux/seq_file.h:11,
                 from arch/powerpc/include/asm/machdep.h:12,
                 from arch/powerpc/include/asm/archrandom.h:7,
                 from include/linux/random.h:166,
                 from include/linux/net.h:22,
                 from include/linux/skbuff.h:29,
                 from include/linux/if_arp.h:26,
                 from include/rdma/ib_addr.h:39,
                 from include/rdma/rdma_cm.h:39,
                 from drivers/infiniband/core/restrack.c:6:
include/linux/xarray.h:525:3: note: expected 'struct xa_limit' but argument is of type 'void *'
   struct xa_limit, gfp_t);
   ^~~~~~~~~~~~~~~
drivers/infiniband/core/restrack.c:29:28: warning: passing argument 3 of '__xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
   err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
                            ^~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/fs.h:15,
                 from include/linux/seq_file.h:11,
                 from arch/powerpc/include/asm/machdep.h:12,
                 from arch/powerpc/include/asm/archrandom.h:7,
                 from include/linux/random.h:166,
                 from include/linux/net.h:22,
                 from include/linux/skbuff.h:29,
                 from include/linux/if_arp.h:26,
                 from include/rdma/ib_addr.h:39,
                 from include/rdma/rdma_cm.h:39,
                 from drivers/infiniband/core/restrack.c:6:
include/linux/xarray.h:524:61: note: expected 'void *' but argument is of type 'u32' {aka 'unsigned int'}
 int __must_check __xa_alloc(struct xarray *, u32 *id, void *entry,
                                                       ~~~~~~^~~~~
drivers/infiniband/core/restrack.c:29:35: error: incompatible type for argument 4 of '__xa_alloc'
   err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
                                   ^~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/fs.h:15,
                 from include/linux/seq_file.h:11,
                 from arch/powerpc/include/asm/machdep.h:12,
                 from arch/powerpc/include/asm/archrandom.h:7,
                 from include/linux/random.h:166,
                 from include/linux/net.h:22,
                 from include/linux/skbuff.h:29,
                 from include/linux/if_arp.h:26,
                 from include/rdma/ib_addr.h:39,
                 from include/rdma/rdma_cm.h:39,
                 from drivers/infiniband/core/restrack.c:6:
include/linux/xarray.h:525:3: note: expected 'struct xa_limit' but argument is of type 'void *'
   struct xa_limit, gfp_t);
   ^~~~~~~~~~~~~~~

Caused by commit

  fd47c2f99f04 ("RDMA/restrack: Convert internal DB from hash to XArray")

from the rdma tree interacting with commit

  a3e4d3f97ec8 ("XArray: Redesign xa_alloc API")

from the xarray tree.

I added the following merge fix patch:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 21 Feb 2019 17:07:22 +1100
Subject: [PATCH] RDMA/restrack: fix for __xa_alloc() API change

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/infiniband/core/restrack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index fa804093fafb..5cb381a986c1 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -23,10 +23,10 @@ static int rt_xa_alloc_cyclic(struct xarray *xa, u32 *id, void *entry,
 		*id = 0;
 
 	xa_lock(xa);
-	err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
+	err = __xa_alloc(xa, id, entry, XA_LIMIT(*id, U32_MAX), GFP_KERNEL);
 	if (err && *next != U32_MAX) {
 		*id = 0;
-		err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
+		err = __xa_alloc(xa, id, entry, XA_LIMIT(0, *next), GFP_KERNEL);
 	}
 
 	if (!err)
-- 
2.20.1

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 05:13:32PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> After merging the xarray tree, today's linux-next build (powerpc
> ppc64_defconfig) failed like this:
>
> In file included from include/linux/uio.h:12,
>                  from include/linux/socket.h:8,
>                  from include/rdma/rdma_cm.h:37,
>                  from drivers/infiniband/core/restrack.c:6:
> drivers/infiniband/core/restrack.c: In function 'rt_xa_alloc_cyclic':
> include/linux/kernel.h:40:18: warning: passing argument 3 of '__xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
>  #define U32_MAX  ((u32)~0U)
>                   ^~~~~~~~~~
> drivers/infiniband/core/restrack.c:26:27: note: in expansion of macro 'U32_MAX'
>   err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
>                            ^~~~~~~
> In file included from include/linux/radix-tree.h:31,
>                  from include/linux/fs.h:15,
>                  from include/linux/seq_file.h:11,
>                  from arch/powerpc/include/asm/machdep.h:12,
>                  from arch/powerpc/include/asm/archrandom.h:7,
>                  from include/linux/random.h:166,
>                  from include/linux/net.h:22,
>                  from include/linux/skbuff.h:29,
>                  from include/linux/if_arp.h:26,
>                  from include/rdma/ib_addr.h:39,
>                  from include/rdma/rdma_cm.h:39,
>                  from drivers/infiniband/core/restrack.c:6:
> include/linux/xarray.h:524:61: note: expected 'void *' but argument is of type 'unsigned int'
>  int __must_check __xa_alloc(struct xarray *, u32 *id, void *entry,
>                                                        ~~~~~~^~~~~
> drivers/infiniband/core/restrack.c:26:36: error: incompatible type for argument 4 of '__xa_alloc'
>   err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
>                                     ^~~~~
> In file included from include/linux/radix-tree.h:31,
>                  from include/linux/fs.h:15,
>                  from include/linux/seq_file.h:11,
>                  from arch/powerpc/include/asm/machdep.h:12,
>                  from arch/powerpc/include/asm/archrandom.h:7,
>                  from include/linux/random.h:166,
>                  from include/linux/net.h:22,
>                  from include/linux/skbuff.h:29,
>                  from include/linux/if_arp.h:26,
>                  from include/rdma/ib_addr.h:39,
>                  from include/rdma/rdma_cm.h:39,
>                  from drivers/infiniband/core/restrack.c:6:
> include/linux/xarray.h:525:3: note: expected 'struct xa_limit' but argument is of type 'void *'
>    struct xa_limit, gfp_t);
>    ^~~~~~~~~~~~~~~
> drivers/infiniband/core/restrack.c:29:28: warning: passing argument 3 of '__xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
>    err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
>                             ^~~~~
> In file included from include/linux/radix-tree.h:31,
>                  from include/linux/fs.h:15,
>                  from include/linux/seq_file.h:11,
>                  from arch/powerpc/include/asm/machdep.h:12,
>                  from arch/powerpc/include/asm/archrandom.h:7,
>                  from include/linux/random.h:166,
>                  from include/linux/net.h:22,
>                  from include/linux/skbuff.h:29,
>                  from include/linux/if_arp.h:26,
>                  from include/rdma/ib_addr.h:39,
>                  from include/rdma/rdma_cm.h:39,
>                  from drivers/infiniband/core/restrack.c:6:
> include/linux/xarray.h:524:61: note: expected 'void *' but argument is of type 'u32' {aka 'unsigned int'}
>  int __must_check __xa_alloc(struct xarray *, u32 *id, void *entry,
>                                                        ~~~~~~^~~~~
> drivers/infiniband/core/restrack.c:29:35: error: incompatible type for argument 4 of '__xa_alloc'
>    err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
>                                    ^~~~~
> In file included from include/linux/radix-tree.h:31,
>                  from include/linux/fs.h:15,
>                  from include/linux/seq_file.h:11,
>                  from arch/powerpc/include/asm/machdep.h:12,
>                  from arch/powerpc/include/asm/archrandom.h:7,
>                  from include/linux/random.h:166,
>                  from include/linux/net.h:22,
>                  from include/linux/skbuff.h:29,
>                  from include/linux/if_arp.h:26,
>                  from include/rdma/ib_addr.h:39,
>                  from include/rdma/rdma_cm.h:39,
>                  from drivers/infiniband/core/restrack.c:6:
> include/linux/xarray.h:525:3: note: expected 'struct xa_limit' but argument is of type 'void *'
>    struct xa_limit, gfp_t);
>    ^~~~~~~~~~~~~~~
>
> Caused by commit
>
>   fd47c2f99f04 ("RDMA/restrack: Convert internal DB from hash to XArray")
>
> from the rdma tree interacting with commit
>
>   a3e4d3f97ec8 ("XArray: Redesign xa_alloc API")
>
> from the xarray tree.

Thanks Stephen, your change looks good.

Matthew, really? change of API in -rc7? And it after you pushed us to
base our -next on -rc5 after another API change? What should we do now?

Can you please ensure that you are sending your pull request to Linus,
after RDMA pull request will be successfully merged?

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi Leon,

On Thu, 21 Feb 2019 12:34:42 +0000 Leon Romanovsky <leonro@mellanox.com> wrote:
>
> Matthew, really? change of API in -rc7? And it after you pushed us to
> base our -next on -rc5 after another API change? What should we do now?

The xarray API changes (to xa_alloc and __xa_alloc) have been in
linux-next for about 2 weeks ...

> Can you please ensure that you are sending your pull request to Linus,
> after RDMA pull request will be successfully merged?

It doesn't matter which goes in first, but whoever goes in last should
inform Linus about these (now 2) API change fixups.  In fact you should
probably both let him know as he sometimes batches up similar groups of
merge requests (like fs or net) and doesn't do them in the order they
are submitted.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Leon Romanovsky]

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

On Thu, Feb 21, 2019 at 11:48:01PM +1100, Stephen Rothwell wrote:
> Hi Leon,
>
> On Thu, 21 Feb 2019 12:34:42 +0000 Leon Romanovsky <leonro@mellanox.com> wrote:
> >
> > Matthew, really? change of API in -rc7? And it after you pushed us to
> > base our -next on -rc5 after another API change? What should we do now?
>
> The xarray API changes (to xa_alloc and __xa_alloc) have been in
> linux-next for about 2 weeks ...

I looked on the latest update of pulled branch
http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray

>
> > Can you please ensure that you are sending your pull request to Linus,
> > after RDMA pull request will be successfully merged?
>
> It doesn't matter which goes in first, but whoever goes in last should
> inform Linus about these (now 2) API change fixups.  In fact you should
> probably both let him know as he sometimes batches up similar groups of
> merge requests (like fs or net) and doesn't do them in the order they
> are submitted.

OK, we will do.

Thanks

>
> --
> Cheers,
> Stephen Rothwell



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi Leon,

On Thu, 21 Feb 2019 13:16:31 +0000 Leon Romanovsky <leonro@mellanox.com> wrote:
>
> > The xarray API changes (to xa_alloc and __xa_alloc) have been in
> > linux-next for about 2 weeks ...  
> 
> I looked on the latest update of pulled branch
> http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray

Yeah, there were 3 commits added today, but the API changing commit is older.

> > It doesn't matter which goes in first, but whoever goes in last should
> > inform Linus about these (now 2) API change fixups.  In fact you should
> > probably both let him know as he sometimes batches up similar groups of
> > merge requests (like fs or net) and doesn't do them in the order they
> > are submitted.  
> 
> OK, we will do.

Thanks.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Thu, Feb 21, 2019 at 12:34:42PM +0000, Leon Romanovsky wrote:
> On Thu, Feb 21, 2019 at 05:13:32PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the xarray tree, today's linux-next build (powerpc
> > ppc64_defconfig) failed like this:

[API change]

So I ended up being really busy last week and not having time to send
my merge request for XArray yet.  I'm going to send a request this week;
Leon and Jason, how does this merge resolution look?

Compile tested only.

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index a9f29156e486..8a93c0c95953 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -668,19 +668,10 @@ static int assign_name(struct ib_device *device, const char *name)
 	}
 	strlcpy(device->name, dev_name(&device->dev), IB_DEVICE_NAME_MAX);
 
-	/* Cyclically allocate a user visible ID for the device */
-	device->index = last_id;
-	ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
-	if (ret == -ENOSPC) {
-		device->index = 0;
-		ret = xa_alloc(&devices, &device->index, INT_MAX, device,
-			       GFP_KERNEL);
-	}
-	if (ret)
-		goto out;
-	last_id = device->index + 1;
-
-	ret = 0;
+	ret = xa_alloc_cyclic(&devices, &device->index, device, xa_limit_31b,
+			&last_id, GFP_KERNEL);
+	if (ret > 0)
+		ret = 0;
 
 out:
 	up_write(&devices_rwsem);
@@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
 	 * to get the LIFO order. The extra linked list can go away if xarray
 	 * learns to reverse iterate.
 	 */
-	if (list_empty(&client_list))
+	if (list_empty(&client_list)) {
 		client->client_id = 0;
-	else
-		client->client_id =
-			list_last_entry(&client_list, struct ib_client, list)
-				->client_id;
-	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
-		       GFP_KERNEL);
+	} else {
+		struct ib_client *last = list_last_entry(&client_list,
+				struct ib_client, list);
+		client->client_id = last->client_id + 1;
+	}
+	ret = xa_insert(&clients, client->client_id, client, GFP_KERNEL);
 	if (ret)
 		goto out;
 
diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index fa804093fafb..3b5ff2f7b5f8 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -13,28 +13,6 @@
 #include "cma_priv.h"
 #include "restrack.h"
 
-static int rt_xa_alloc_cyclic(struct xarray *xa, u32 *id, void *entry,
-			      u32 *next)
-{
-	int err;
-
-	*id = *next;
-	if (*next == U32_MAX)
-		*id = 0;
-
-	xa_lock(xa);
-	err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
-	if (err && *next != U32_MAX) {
-		*id = 0;
-		err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
-	}
-
-	if (!err)
-		*next = *id + 1;
-	xa_unlock(xa);
-	return err;
-}
-
 /**
  * rdma_restrack_init() - initialize and allocate resource tracking
  * @dev:  IB device
@@ -226,7 +204,8 @@ static void rdma_restrack_add(struct rdma_restrack_entry *res)
 	kref_init(&res->kref);
 	init_completion(&res->comp);
 	if (res->type != RDMA_RESTRACK_QP)
-		ret = rt_xa_alloc_cyclic(&rt->xa, &res->id, res, &rt->next_id);
+		ret = xa_alloc_cyclic(&rt->xa, &res->id, res, xa_limit_32b,
+				&rt->next_id, GFP_KERNEL);
 	else {
 		/* Special case to ensure that LQPN points to right QP */
 		struct ib_qp *qp = container_of(res, struct ib_qp, res);

Re: linux-next: build failure after merge of the xarray tree

[Jason Gunthorpe]

On Sun, Mar 10, 2019 at 07:44:34PM -0700, Matthew Wilcox wrote:
> On Thu, Feb 21, 2019 at 12:34:42PM +0000, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 05:13:32PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > After merging the xarray tree, today's linux-next build (powerpc
> > > ppc64_defconfig) failed like this:
> 
> [API change]
> 
> So I ended up being really busy last week and not having time to send
> my merge request for XArray yet.  I'm going to send a request this week;
> Leon and Jason, how does this merge resolution look?
> 
> Compile tested only.
> 
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a9f29156e486..8a93c0c95953 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -668,19 +668,10 @@ static int assign_name(struct ib_device *device, const char *name)
>  	}
>  	strlcpy(device->name, dev_name(&device->dev), IB_DEVICE_NAME_MAX);
>  
> -	/* Cyclically allocate a user visible ID for the device */
> -	device->index = last_id;
> -	ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
> -	if (ret == -ENOSPC) {
> -		device->index = 0;
> -		ret = xa_alloc(&devices, &device->index, INT_MAX, device,
> -			       GFP_KERNEL);
> -	}
> -	if (ret)
> -		goto out;
> -	last_id = device->index + 1;
> -
> -	ret = 0;
> +	ret = xa_alloc_cyclic(&devices, &device->index, device, xa_limit_31b,
> +			&last_id, GFP_KERNEL);
> +	if (ret > 0)
> +		ret = 0;
>  
>  out:
>  	up_write(&devices_rwsem);
> @@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
>  	 * to get the LIFO order. The extra linked list can go away if xarray
>  	 * learns to reverse iterate.
>  	 */
> -	if (list_empty(&client_list))
> +	if (list_empty(&client_list)) {
>  		client->client_id = 0;
> -	else
> -		client->client_id =
> -			list_last_entry(&client_list, struct ib_client, list)
> -				->client_id;
> -	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
> -		       GFP_KERNEL);
> +	} else {
> +		struct ib_client *last = list_last_entry(&client_list,
> +				struct ib_client, list);
> +		client->client_id = last->client_id + 1;

blank line after locals, but other wise these all looks fine.. 

Should have started out with the xa_insert version above..

Thanks,
Jason

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Mon, Mar 11, 2019 at 12:13:54PM +0000, Jason Gunthorpe wrote:
> > @@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
> >  	 * to get the LIFO order. The extra linked list can go away if xarray
> >  	 * learns to reverse iterate.
> >  	 */
> > -	if (list_empty(&client_list))
> > +	if (list_empty(&client_list)) {
> >  		client->client_id = 0;
> > -	else
> > -		client->client_id =
> > -			list_last_entry(&client_list, struct ib_client, list)
> > -				->client_id;
> > -	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
> > -		       GFP_KERNEL);
> > +	} else {
> > +		struct ib_client *last = list_last_entry(&client_list,
> > +				struct ib_client, list);
> > +		client->client_id = last->client_id + 1;
> 
> blank line after locals, but other wise these all looks fine.. 

Would you rather see this rendered as:

        if (list_empty(&client_list)) {
                client->client_id = 0;
        } else {
                struct ib_client *last; 

                last = list_last_entry(&client_list, struct ib_client, list);
                client->client_id = last->client_id + 1;
        }

or move the declaration of 'last' up to the top of the function?

> Should have started out with the xa_insert version above..

I didn't spot it until last night either ...

Re: linux-next: build failure after merge of the xarray tree

[Leon Romanovsky]

On Sun, Mar 10, 2019 at 07:44:34PM -0700, Matthew Wilcox wrote:
> On Thu, Feb 21, 2019 at 12:34:42PM +0000, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 05:13:32PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > After merging the xarray tree, today's linux-next build (powerpc
> > > ppc64_defconfig) failed like this:
>
> [API change]
>
> So I ended up being really busy last week and not having time to send
> my merge request for XArray yet.  I'm going to send a request this week;
> Leon and Jason, how does this merge resolution look?
>
> Compile tested only.
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index a9f29156e486..8a93c0c95953 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -668,19 +668,10 @@ static int assign_name(struct ib_device *device, const char *name)
>  	}
>  	strlcpy(device->name, dev_name(&device->dev), IB_DEVICE_NAME_MAX);
>
> -	/* Cyclically allocate a user visible ID for the device */
> -	device->index = last_id;
> -	ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
> -	if (ret == -ENOSPC) {
> -		device->index = 0;
> -		ret = xa_alloc(&devices, &device->index, INT_MAX, device,
> -			       GFP_KERNEL);
> -	}
> -	if (ret)
> -		goto out;
> -	last_id = device->index + 1;
> -
> -	ret = 0;
> +	ret = xa_alloc_cyclic(&devices, &device->index, device, xa_limit_31b,
> +			&last_id, GFP_KERNEL);
> +	if (ret > 0)
> +		ret = 0;
>
>  out:
>  	up_write(&devices_rwsem);
> @@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
>  	 * to get the LIFO order. The extra linked list can go away if xarray
>  	 * learns to reverse iterate.
>  	 */
> -	if (list_empty(&client_list))
> +	if (list_empty(&client_list)) {
>  		client->client_id = 0;
> -	else
> -		client->client_id =
> -			list_last_entry(&client_list, struct ib_client, list)
> -				->client_id;
> -	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
> -		       GFP_KERNEL);
> +	} else {
> +		struct ib_client *last = list_last_entry(&client_list,
> +				struct ib_client, list);
> +		client->client_id = last->client_id + 1;
> +	}
> +	ret = xa_insert(&clients, client->client_id, client, GFP_KERNEL);
>  	if (ret)
>  		goto out;
>
> diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> index fa804093fafb..3b5ff2f7b5f8 100644
> --- a/drivers/infiniband/core/restrack.c
> +++ b/drivers/infiniband/core/restrack.c
> @@ -13,28 +13,6 @@
>  #include "cma_priv.h"
>  #include "restrack.h"
>
> -static int rt_xa_alloc_cyclic(struct xarray *xa, u32 *id, void *entry,
> -			      u32 *next)
> -{
> -	int err;
> -
> -	*id = *next;
> -	if (*next == U32_MAX)
> -		*id = 0;
> -
> -	xa_lock(xa);
> -	err = __xa_alloc(xa, id, U32_MAX, entry, GFP_KERNEL);
> -	if (err && *next != U32_MAX) {
> -		*id = 0;
> -		err = __xa_alloc(xa, id, *next, entry, GFP_KERNEL);
> -	}
> -
> -	if (!err)
> -		*next = *id + 1;
> -	xa_unlock(xa);
> -	return err;
> -}
> -
>  /**
>   * rdma_restrack_init() - initialize and allocate resource tracking
>   * @dev:  IB device
> @@ -226,7 +204,8 @@ static void rdma_restrack_add(struct rdma_restrack_entry *res)
>  	kref_init(&res->kref);
>  	init_completion(&res->comp);
>  	if (res->type != RDMA_RESTRACK_QP)
> -		ret = rt_xa_alloc_cyclic(&rt->xa, &res->id, res, &rt->next_id);
> +		ret = xa_alloc_cyclic(&rt->xa, &res->id, res, xa_limit_32b,
> +				&rt->next_id, GFP_KERNEL);

restrack.c part looks good.
Thanks


>  	else {
>  		/* Special case to ensure that LQPN points to right QP */
>  		struct ib_qp *qp = container_of(res, struct ib_qp, res);

Re: linux-next: build failure after merge of the xarray tree

[Jason Gunthorpe]

On Mon, Mar 11, 2019 at 05:31:05AM -0700, Matthew Wilcox wrote:
> On Mon, Mar 11, 2019 at 12:13:54PM +0000, Jason Gunthorpe wrote:
> > > @@ -1059,14 +1050,14 @@ static int assign_client_id(struct ib_client *client)
> > >  	 * to get the LIFO order. The extra linked list can go away if xarray
> > >  	 * learns to reverse iterate.
> > >  	 */
> > > -	if (list_empty(&client_list))
> > > +	if (list_empty(&client_list)) {
> > >  		client->client_id = 0;
> > > -	else
> > > -		client->client_id =
> > > -			list_last_entry(&client_list, struct ib_client, list)
> > > -				->client_id;
> > > -	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
> > > -		       GFP_KERNEL);
> > > +	} else {
> > > +		struct ib_client *last = list_last_entry(&client_list,
> > > +				struct ib_client, list);
> > > +		client->client_id = last->client_id + 1;
> > 
> > blank line after locals, but other wise these all looks fine.. 
> 
> Would you rather see this rendered as:
> 
>         if (list_empty(&client_list)) {
>                 client->client_id = 0;
>         } else {
>                 struct ib_client *last; 
> 
>                 last = list_last_entry(&client_list, struct ib_client, list);
>                 client->client_id = last->client_id + 1;
>         }

Don't care much either way. Only that the Linux style guide is to
always have the blank line after variable declarations in any block

> or move the declaration of 'last' up to the top of the function?

This one I dislike, variables should be in their narrowest scope for
clarity

> > Should have started out with the xa_insert version above..
> 
> I didn't spot it until last night either ...

It is a leftover weird thinking logic from an earlier attempt I had
that was trying to get the last ID out of the xarray without the
linked list..

Jason

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Thu, Oct 08, 2020 at 05:55:40PM +1100, Stephen Rothwell wrote:
> After merging the xarray tree, today's linux-next build (x86_64
> allmodconfig) failed like this:

Thanks, fixed both problems.

linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

After merging the xarray tree, today's linux-next build (x86_64
allmodconfig) failed like this:

ERROR: modpost: "xa_delete_node" [lib/test_xarray.ko] undefined!

Caused by commit

  e95150e70fe1 ("XArray: Add private interface for workingset node deletion")

I have added the following hack for today:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 8 Oct 2020 17:52:11 +1100
Subject: [PATCH] XArray: export xa_delete_node()

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 lib/xarray.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/xarray.c b/lib/xarray.c
index 3f0b143bfdcd..be9166b45db3 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1993,6 +1993,7 @@ void xa_delete_node(struct xa_node *node, xa_update_node_t update)
 
 	xas_store(&xas, NULL);
 }
+EXPORT_SYMBOL(xa_delete_node);
 
 /**
  * xa_destroy() - Free all internal data structures.
-- 
2.28.0

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

After merging the xarray tree, today's linux-next build (x86_64
allmodconfig) failed like this:

lib/test_xarray.c: In function 'check_xa_mark_3':
lib/test_xarray.c:305:3: error: implicit declaration of function 'assert' [-Werror=implicit-function-declaration]
  305 |   assert(1);
      |   ^~~~~~
lib/test_xarray.c:11:1: note: 'assert' is defined in header '<assert.h>'; did you forget to '#include <assert.h>'?
   10 | #include <linux/module.h>
  +++ |+#include <assert.h>
   11 | 

Caused by commit

  5c8052d7925b ("XArray test: Add new test")

I have added the following hack for today:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 8 Oct 2020 17:46:26 +1100
Subject: [PATCH] XArray test: remove assert()

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 lib/test_xarray.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/test_xarray.c b/lib/test_xarray.c
index 9d9c09d1f781..21bb06c213a2 100644
--- a/lib/test_xarray.c
+++ b/lib/test_xarray.c
@@ -302,7 +302,6 @@ static noinline void check_xa_mark_3(struct xarray *xa)
 	rcu_read_lock();
 	xas_for_each_marked(&xas, entry, ULONG_MAX, XA_MARK_0) {
 		count++;
-		assert(1);
 	}
 	XA_BUG_ON(xa, count != 1);
 	rcu_read_unlock();
-- 
2.28.0

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Jason Gunthorpe]

On Thu, Feb 21, 2019 at 11:47:16PM +1100, Stephen Rothwell wrote:
> Hi Jason,
> 
> On Wed, 13 Feb 2019 22:09:36 +0000 Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > I personally think it is not good to put major logic changes in merge
> > commits, so I would prefer the #2 approach for this case.
> 
> These are not difficult merge fixes or logic changes.

That comment was directed at Matt's correct suggestion to use
alloc_cyclic, this same suggestion applies to this conflict as well.

The mechanical change of the args is not so bad for a merge commit.

> > SFR's tree is just a reference. Who ever takes care to resolve these
> > conflicts has to manually do the fixing up. If you do send your tree
> > early I will fix it up as part of prepping the RDMA tree PR. Otherwise
> > you will have to fix it.
> 
> Neither of you need to fix it ...

Not quite, I always re-do and verify all your merge resolutions and
send them to Linus as both a raw diff and as an actual merge commit
for him to reference. This way these things are checked in multiple
ways.

It is difficult to run this flow if we don't have an agreed order of
trees, as producing the resolutions for an unmerged tree is not so
straight forward.

Jason

Re: linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi Jason,

On Wed, 13 Feb 2019 22:09:36 +0000 Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> I personally think it is not good to put major logic changes in merge
> commits, so I would prefer the #2 approach for this case.

These are not difficult merge fixes or logic changes.

> Also, the general philosophy that the person doing the tree-wide
> change should do the work :)

In fact, I have done the work :-)  All you guys have to do is inform
Linus and give him my resolutions.  The change to xa_alloc_cyclic could
just be a followup patch once the air clears.

> SFR's tree is just a reference. Who ever takes care to resolve these
> conflicts has to manually do the fixing up. If you do send your tree
> early I will fix it up as part of prepping the RDMA tree PR. Otherwise
> you will have to fix it.

Neither of you need to fix it ...

> What I don't want to see is we send both trees at the same time and
> neither gives merge guidance to Linus.

Well, I have now reminded you both, so hopefully you will both tell
Linus.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Jason Gunthorpe]

On Wed, Feb 13, 2019 at 01:26:23PM -0800, Matthew Wilcox wrote:
> On Tue, Feb 12, 2019 at 04:23:31PM +0000, Jason Gunthorpe wrote:
> > On Tue, Feb 12, 2019 at 08:15:28AM -0800, Matthew Wilcox wrote:
> > > Seriously, there are several defects in the published API which do
> > > warrant a change.  The most severe one is that it's really easy to
> > > forget to initialise the start index.  And while I'm making that change,
> > > I should fix smaller things like the errno at the same time.
> > 
> > I hope you will send your tree in the 2nd week of the merge window
> > with all these merge fixes in it..
> > 
> > I think Linus will not like it if he has to fix this when merging
> > rdma.
> 
> Ahhahhahhah.  No.  Burned once.  Not doing that again.

I'm not suggesting you should do that - basing on the nvdimm tree to
avoid conflicts was a terrible idea.

There are two sane approaches you can do here:

1) Base on -rcX and Merge to Linus's tree and resolve all conflicts
   before sending the PR. The PR should ideally go in the second week
   as it is a tree wide API change.

   You can read Linus's comments on handling complex merge resolutions
   in PRs here:
    https://lore.kernel.org/lkml/CA+55aFzcp=a2sNOVR7DTEY9dehYamXaJ1bpSfF0FZir_9MhsVg@mail.gmail.com/

2) Rebase on top of Linus's actual tree in the *second* week of the
   merge window and resolve all conflicts in your rebase, then
   send the PR.

In both cases coordinate with conflicting trees to have their PR's
sent in the first week so Linus sees no conflicts.
  
This is basically the same advise Linus gave you:

https://lore.kernel.org/lkml/CA+55aFw+dwofadgvzrM-UCMSih+f1choCwW+xFFM3aPjoRQX_g@mail.gmail.com/

Linus talks here about the trade off for #1 and #2, with a preference
to #1.

#2 is good for "you just want your series to make more sense on its
own" - which I think is where tree-wide changes tend to end up.

I personally think it is not good to put major logic changes in merge
commits, so I would prefer the #2 approach for this case.

Also, the general philosophy that the person doing the tree-wide
change should do the work :)

> > > @@ -750,7 +738,7 @@ int ib_register_device(struct ib_device *device, const char *name)
> > >  	int ret;
> > >  
> > >  	ret = assign_name(device, name);
> > > -	if (ret)
> > > +	if (ret < 0)
> > >  		return ret;
> > 
> > This <0 should be near the xa_alloc_cyclic, I don't want the unusual
> > '1' to propogate.. Far too likely that someone will forget about
> > the special case.
> 
> Feel free to propose an alternate fix for sfr to put in his tree and we
> can both include it as a proposed patch in our respective pull requests.

SFR's tree is just a reference. Who ever takes care to resolve these
conflicts has to manually do the fixing up. If you do send your tree
early I will fix it up as part of prepping the RDMA tree PR. Otherwise
you will have to fix it.

What I don't want to see is we send both trees at the same time and
neither gives merge guidance to Linus.

Also, FWIW, there are two more series in the RDMA patchworks adding an
xa_array call.

Regards,
Jason

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Tue, Feb 12, 2019 at 04:23:31PM +0000, Jason Gunthorpe wrote:
> On Tue, Feb 12, 2019 at 08:15:28AM -0800, Matthew Wilcox wrote:
> > Seriously, there are several defects in the published API which do
> > warrant a change.  The most severe one is that it's really easy to
> > forget to initialise the start index.  And while I'm making that change,
> > I should fix smaller things like the errno at the same time.
> 
> I hope you will send your tree in the 2nd week of the merge window
> with all these merge fixes in it..
> 
> I think Linus will not like it if he has to fix this when merging
> rdma.

Ahhahhahhah.  No.  Burned once.  Not doing that again.

https://lore.kernel.org/lkml/CA+55aFxFjAmrFpwQmEHCthHOzgidCKnod+cNDEE+3Spu9o1s3w@mail.gmail.com/

> > @@ -750,7 +738,7 @@ int ib_register_device(struct ib_device *device, const char *name)
> >  	int ret;
> >  
> >  	ret = assign_name(device, name);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> 
> This <0 should be near the xa_alloc_cyclic, I don't want the unusual
> '1' to propogate.. Far too likely that someone will forget about
> the special case.

Feel free to propose an alternate fix for sfr to put in his tree and we
can both include it as a proposed patch in our respective pull requests.

Re: linux-next: build failure after merge of the xarray tree

[Jason Gunthorpe]

On Tue, Feb 12, 2019 at 08:15:28AM -0800, Matthew Wilcox wrote:
> On Tue, Feb 12, 2019 at 04:20:03PM +1100, Stephen Rothwell wrote:
> > Caused by commit
> > 
> >   a3e4d3f97ec8 ("XArray: Redesign xa_alloc API")
> > 
> > interacting with commits
> > 
> >   e59178d895af ("RDMA/devices: Use xarray to store the clients")
> >   0df91bb67334 ("RDMA/devices: Use xarray to store the client_data")
> > 
> > from the rdma tree.
> > 
> > Its a bit of a pain modifying a published API like this :-(
> 
> Yes, it is.  I wasn't expecting people to actually start using it ;-)
> 
> Seriously, there are several defects in the published API which do
> warrant a change.  The most severe one is that it's really easy to
> forget to initialise the start index.  And while I'm making that change,
> I should fix smaller things like the errno at the same time.

I hope you will send your tree in the 2nd week of the merge window
with all these merge fixes in it..

I think Linus will not like it if he has to fix this when merging
rdma.

> > I have added the following merge fixup patch for today (I assume some
> > of the assignments are also now redundant).
>
> I think the first of these should be using the alloc_cyclic API, like this:

Yes, it is waiting for you :)

> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 283ecc2aee89..d0b56c70a553 100644
> +++ b/drivers/infiniband/core/device.c
> @@ -586,20 +586,8 @@ static int assign_name(struct ib_device *device, const char *name)
>  	}
>  	strlcpy(device->name, dev_name(&device->dev), IB_DEVICE_NAME_MAX);
>  
> -	/* Cyclically allocate a user visible ID for the device */
> -	device->index = last_id;
> -	ret = xa_alloc(&devices, &device->index, device,
> -		       XA_LIMIT(last_id, INT_MAX), GFP_KERNEL);
> -	if (ret == -ENOSPC) {
> -		device->index = 0;
> -		ret = xa_alloc(&devices, &device->index, device,
> -			       XA_LIMIT(0, INT_MAX), GFP_KERNEL);
> -	}
> -	if (ret)
> -		goto out;
> -	last_id = device->index + 1;
> -
> -	ret = 0;
> +	ret = xa_alloc_cyclic(&devices, &device->index, device, xa_limit_31b,
> +			&last_id, GFP_KERNEL);
>  
>  out:
>  	up_write(&devices_rwsem);
> @@ -750,7 +738,7 @@ int ib_register_device(struct ib_device *device, const char *name)
>  	int ret;
>  
>  	ret = assign_name(device, name);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;

This <0 should be near the xa_alloc_cyclic, I don't want the unusual
'1' to propogate.. Far too likely that someone will forget about
the special case.

Jason

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Tue, Feb 12, 2019 at 04:20:03PM +1100, Stephen Rothwell wrote:
> Caused by commit
> 
>   a3e4d3f97ec8 ("XArray: Redesign xa_alloc API")
> 
> interacting with commits
> 
>   e59178d895af ("RDMA/devices: Use xarray to store the clients")
>   0df91bb67334 ("RDMA/devices: Use xarray to store the client_data")
> 
> from the rdma tree.
> 
> Its a bit of a pain modifying a published API like this :-(

Yes, it is.  I wasn't expecting people to actually start using it ;-)

Seriously, there are several defects in the published API which do
warrant a change.  The most severe one is that it's really easy to
forget to initialise the start index.  And while I'm making that change,
I should fix smaller things like the errno at the same time.

> I have added the following merge fixup patch for today (I assume some
> of the assignments are also now redundant).

I think the first of these should be using the alloc_cyclic API, like this:

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 283ecc2aee89..d0b56c70a553 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -586,20 +586,8 @@ static int assign_name(struct ib_device *device, const char *name)
 	}
 	strlcpy(device->name, dev_name(&device->dev), IB_DEVICE_NAME_MAX);
 
-	/* Cyclically allocate a user visible ID for the device */
-	device->index = last_id;
-	ret = xa_alloc(&devices, &device->index, device,
-		       XA_LIMIT(last_id, INT_MAX), GFP_KERNEL);
-	if (ret == -ENOSPC) {
-		device->index = 0;
-		ret = xa_alloc(&devices, &device->index, device,
-			       XA_LIMIT(0, INT_MAX), GFP_KERNEL);
-	}
-	if (ret)
-		goto out;
-	last_id = device->index + 1;
-
-	ret = 0;
+	ret = xa_alloc_cyclic(&devices, &device->index, device, xa_limit_31b,
+			&last_id, GFP_KERNEL);
 
 out:
 	up_write(&devices_rwsem);
@@ -750,7 +738,7 @@ int ib_register_device(struct ib_device *device, const char *name)
 	int ret;
 
 	ret = assign_name(device, name);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	ret = setup_device(device);

linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

After merging the xarray tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

In file included from include/linux/list.h:9,
                 from include/linux/module.h:9,
                 from drivers/infiniband/core/device.c:34:
drivers/infiniband/core/device.c: In function 'assign_name':
include/linux/kernel.h:22:18: warning: passing argument 3 of 'xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
 #define INT_MAX  ((int)(~0U>>1))
                  ^~~~~~~~~~~~~~~
drivers/infiniband/core/device.c:591:43: note: in expansion of macro 'INT_MAX'
  ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
                                           ^~~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:9: note: expected 'void *' but argument is of type 'int'
   void *entry, struct xa_limit limit, gfp_t gfp)
   ~~~~~~^~~~~
drivers/infiniband/core/device.c:591:52: error: incompatible type for argument 4 of 'xa_alloc'
  ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
                                                    ^~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:32: note: expected 'struct xa_limit' but argument is of type 'struct ib_device *'
   void *entry, struct xa_limit limit, gfp_t gfp)
                ~~~~~~~~~~~~~~~~^~~~~
In file included from include/linux/list.h:9,
                 from include/linux/module.h:9,
                 from drivers/infiniband/core/device.c:34:
include/linux/kernel.h:22:18: warning: passing argument 3 of 'xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
 #define INT_MAX  ((int)(~0U>>1))
                  ^~~~~~~~~~~~~~~
drivers/infiniband/core/device.c:594:44: note: in expansion of macro 'INT_MAX'
   ret = xa_alloc(&devices, &device->index, INT_MAX, device,
                                            ^~~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:9: note: expected 'void *' but argument is of type 'int'
   void *entry, struct xa_limit limit, gfp_t gfp)
   ~~~~~~^~~~~
drivers/infiniband/core/device.c:594:53: error: incompatible type for argument 4 of 'xa_alloc'
   ret = xa_alloc(&devices, &device->index, INT_MAX, device,
                                                     ^~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:32: note: expected 'struct xa_limit' but argument is of type 'struct ib_device *'
   void *entry, struct xa_limit limit, gfp_t gfp)
                ~~~~~~~~~~~~~~~~^~~~~
In file included from include/linux/list.h:9,
                 from include/linux/module.h:9,
                 from drivers/infiniband/core/device.c:34:
drivers/infiniband/core/device.c: In function 'assign_client_id':
include/linux/kernel.h:22:18: warning: passing argument 3 of 'xa_alloc' makes pointer from integer without a cast [-Wint-conversion]
 #define INT_MAX  ((int)(~0U>>1))
                  ^~~~~~~~~~~~~~~
drivers/infiniband/core/device.c:826:47: note: in expansion of macro 'INT_MAX'
  ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
                                               ^~~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:9: note: expected 'void *' but argument is of type 'int'
   void *entry, struct xa_limit limit, gfp_t gfp)
   ~~~~~~^~~~~
drivers/infiniband/core/device.c:826:56: error: incompatible type for argument 4 of 'xa_alloc'
  ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
                                                        ^~~~~~
In file included from include/linux/radix-tree.h:31,
                 from include/linux/idr.h:15,
                 from include/linux/kernfs.h:14,
                 from include/linux/sysfs.h:16,
                 from include/linux/kobject.h:20,
                 from include/linux/module.h:17,
                 from drivers/infiniband/core/device.c:34:
include/linux/xarray.h:796:32: note: expected 'struct xa_limit' but argument is of type 'struct ib_client *'
   void *entry, struct xa_limit limit, gfp_t gfp)
                ~~~~~~~~~~~~~~~~^~~~~

Caused by commit

  a3e4d3f97ec8 ("XArray: Redesign xa_alloc API")

interacting with commits

  e59178d895af ("RDMA/devices: Use xarray to store the clients")
  0df91bb67334 ("RDMA/devices: Use xarray to store the client_data")

from the rdma tree.

Its a bit of a pain modifying a published API like this :-(

I have added the following merge fixup patch for today (I assume some
of the assignments are also now redundant).

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 12 Feb 2019 16:09:58 +1100
Subject: [PATCH] RDMA/devices: fix up for xa_alloc API change

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/infiniband/core/device.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 3325be4f91a5..283ecc2aee89 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -588,11 +588,12 @@ static int assign_name(struct ib_device *device, const char *name)
 
 	/* Cyclically allocate a user visible ID for the device */
 	device->index = last_id;
-	ret = xa_alloc(&devices, &device->index, INT_MAX, device, GFP_KERNEL);
+	ret = xa_alloc(&devices, &device->index, device,
+		       XA_LIMIT(last_id, INT_MAX), GFP_KERNEL);
 	if (ret == -ENOSPC) {
 		device->index = 0;
-		ret = xa_alloc(&devices, &device->index, INT_MAX, device,
-			       GFP_KERNEL);
+		ret = xa_alloc(&devices, &device->index, device,
+			       XA_LIMIT(0, INT_MAX), GFP_KERNEL);
 	}
 	if (ret)
 		goto out;
@@ -823,8 +824,8 @@ static int assign_client_id(struct ib_client *client)
 		client->client_id =
 			list_last_entry(&client_list, struct ib_client, list)
 				->client_id;
-	ret = xa_alloc(&clients, &client->client_id, INT_MAX, client,
-		       GFP_KERNEL);
+	ret = xa_alloc(&clients, &client->client_id, client,
+		       XA_LIMIT(client->client_id, INT_MAX), GFP_KERNEL);
 	if (ret)
 		goto out;
 
-- 
2.20.1

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

On Mon, 18 Jun 2018 10:09:20 -0700 Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Jun 18, 2018 at 09:50:33AM -0700, Dan Williams wrote:
> > On Sun, Jun 17, 2018 at 8:27 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:  
> > > Hi all,
> > >
> > > After merging the xarray tree, today's linux-next build (powerpc
> > > ppc64_defconfig) failed like this:  
> [...]
> > > from the nvdimm tree.
> > >
> > > Willy thanks for the heads up about this.
> > >
> > > I have applied the following merge fix patch (taken from the diff between
> > > the -next tree at this point and the xarray-20180615 branch from the
> > > xarray tree) for today.  
> > 
> > I was hoping that dax_lock_page() and the memory_failure() handling
> > could go in before the xarray rework. This helps -stable and distros
> > that need to backport this error handling support. Willy, would you be
> > amenable to rebasing on top of the next rev of the
> > dax+memory_failure() work?
> > 
> > Apologies for the thrash.  
> 
> I am absolutely amenable to rebasing.  The only problem is that I'm
> in Tokyo for the next two weeks.  I can put some work in on this, but
> coordination may be a little off.  If somebody else wants to do the work,
> the only (serious) difference between the xarray-20180615 and xarray
> branches in my repo is that the former is based on the dax_lock_page()
> changes having gone in.
> 
> The differences sum up to:
> 
> +@@ -414,8 +413,7 @@ struct page *dax_lock_page(unsigned long pfn)
> + 
> +               entry = __radix_tree_lookup(&mapping->i_pages, index, NULL,
> +                               &slot);
> +-              if (!entry ||
> +-                  WARN_ON_ONCE(!radix_tree_exceptional_entry(entry))) {
> ++              if (!entry || WARN_ON_ONCE(!xa_is_value(entry))) {
> +                       xa_unlock_irq(&mapping->i_pages);
> +                       break;
> +               } else if (!slot_locked(mapping, slot)) {
> 
> (in "xarray: Replace exceptional entries")
> 
> then dax_entry_waitqueue() changing its argument in "dax: Hash on XArray instead of mapping".
> 
> and finally the patch converting dax_lock_page() and dax_unlock_page().
> 
> I really wanted to keep the thrash here to a minimum, but this is the
> best I could come up with in terms of minimising conflicts :-(

This has all gone away today as the conflicting commits have been
removed from the nvdimm tree.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build failure after merge of the xarray tree

[Matthew Wilcox]

On Mon, Jun 18, 2018 at 09:50:33AM -0700, Dan Williams wrote:
> On Sun, Jun 17, 2018 at 8:27 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > Hi all,
> >
> > After merging the xarray tree, today's linux-next build (powerpc
> > ppc64_defconfig) failed like this:
[...]
> > from the nvdimm tree.
> >
> > Willy thanks for the heads up about this.
> >
> > I have applied the following merge fix patch (taken from the diff between
> > the -next tree at this point and the xarray-20180615 branch from the
> > xarray tree) for today.
> 
> I was hoping that dax_lock_page() and the memory_failure() handling
> could go in before the xarray rework. This helps -stable and distros
> that need to backport this error handling support. Willy, would you be
> amenable to rebasing on top of the next rev of the
> dax+memory_failure() work?
> 
> Apologies for the thrash.

I am absolutely amenable to rebasing.  The only problem is that I'm
in Tokyo for the next two weeks.  I can put some work in on this, but
coordination may be a little off.  If somebody else wants to do the work,
the only (serious) difference between the xarray-20180615 and xarray
branches in my repo is that the former is based on the dax_lock_page()
changes having gone in.

The differences sum up to:

+@@ -414,8 +413,7 @@ struct page *dax_lock_page(unsigned long pfn)
+ 
+               entry = __radix_tree_lookup(&mapping->i_pages, index, NULL,
+                               &slot);
+-              if (!entry ||
+-                  WARN_ON_ONCE(!radix_tree_exceptional_entry(entry))) {
++              if (!entry || WARN_ON_ONCE(!xa_is_value(entry))) {
+                       xa_unlock_irq(&mapping->i_pages);
+                       break;
+               } else if (!slot_locked(mapping, slot)) {

(in "xarray: Replace exceptional entries")

then dax_entry_waitqueue() changing its argument in "dax: Hash on XArray instead of mapping".

and finally the patch converting dax_lock_page() and dax_unlock_page().

I really wanted to keep the thrash here to a minimum, but this is the
best I could come up with in terms of minimising conflicts :-(

Re: linux-next: build failure after merge of the xarray tree

[Dan Williams]

On Sun, Jun 17, 2018 at 8:27 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> Hi all,
>
> After merging the xarray tree, today's linux-next build (powerpc
> ppc64_defconfig) failed like this:
>
> In file included from arch/powerpc/include/asm/bug.h:128:0,
>                  from include/linux/bug.h:5,
>                  from arch/powerpc/include/asm/cmpxchg.h:9,
>                  from arch/powerpc/include/asm/atomic.h:11,
>                  from include/linux/atomic.h:5,
>                  from fs/dax.c:17:
> fs/dax.c: In function 'dax_lock_page':
> fs/dax.c:392:21: error: implicit declaration of function 'radix_tree_exceptional_entry'; did you mean 'radix_tree_exception'? [-Werror=implicit-function-declaration]
>        WARN_ON_ONCE(!radix_tree_exceptional_entry(entry))) {
>                      ^
> include/asm-generic/bug.h:69:25: note: in definition of macro 'WARN_ON_ONCE'
>   int __ret_warn_on = !!(condition);   \
>                          ^~~~~~~~~
> fs/dax.c:395:15: error: implicit declaration of function 'slot_locked'; did you mean 'iget_locked'? [-Werror=implicit-function-declaration]
>    } else if (!slot_locked(mapping, slot)) {
>                ^~~~~~~~~~~
>                iget_locked
> fs/dax.c:396:4: error: implicit declaration of function 'lock_slot'; did you mean 'local_set'? [-Werror=implicit-function-declaration]
>     lock_slot(mapping, slot);
>     ^~~~~~~~~
>     local_set
> fs/dax.c:402:28: error: passing argument 1 of 'dax_entry_waitqueue' from incompatible pointer type [-Werror=incompatible-pointer-types]
>    wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
>                             ^~~~~~~
> fs/dax.c:152:27: note: expected 'struct xa_state *' but argument is of type 'struct address_space *'
>  static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
>                            ^~~~~~~~~~~~~~~~~~~
> fs/dax.c:402:37: warning: passing argument 2 of 'dax_entry_waitqueue' makes pointer from integer without a cast [-Wint-conversion]
>    wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
>                                      ^~~~~
> fs/dax.c:152:27: note: expected 'void *' but argument is of type 'long unsigned int'
>  static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
>                            ^~~~~~~~~~~~~~~~~~~
> fs/dax.c:402:8: error: too many arguments to function 'dax_entry_waitqueue'
>    wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
>         ^~~~~~~~~~~~~~~~~~~
> fs/dax.c:152:27: note: declared here
>  static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
>                            ^~~~~~~~~~~~~~~~~~~
> fs/dax.c: In function 'dax_unlock_page':
> fs/dax.c:424:2: error: implicit declaration of function 'dax_unlock_mapping_entry'; did you mean 'dax_delete_mapping_entry'? [-Werror=implicit-function-declaration]
>   dax_unlock_mapping_entry(mapping, page->index);
>   ^~~~~~~~~~~~~~~~~~~~~~~~
>   dax_delete_mapping_entry
>
> Caused by commits in the xarray tree interacting with commit
>
>   9b3d53936caa ("filesystem-dax: Introduce dax_lock_page()")
>
> from the nvdimm tree.
>
> Willy thanks for the heads up about this.
>
> I have applied the following merge fix patch (taken from the diff between
> the -next tree at this point and the xarray-20180615 branch from the
> xarray tree) for today.

I was hoping that dax_lock_page() and the memory_failure() handling
could go in before the xarray rework. This helps -stable and distros
that need to backport this error handling support. Willy, would you be
amenable to rebasing on top of the next rev of the
dax+memory_failure() work?

Apologies for the thrash.

linux-next: build failure after merge of the xarray tree

[Stephen Rothwell]

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

Hi all,

After merging the xarray tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

In file included from arch/powerpc/include/asm/bug.h:128:0,
                 from include/linux/bug.h:5,
                 from arch/powerpc/include/asm/cmpxchg.h:9,
                 from arch/powerpc/include/asm/atomic.h:11,
                 from include/linux/atomic.h:5,
                 from fs/dax.c:17:
fs/dax.c: In function 'dax_lock_page':
fs/dax.c:392:21: error: implicit declaration of function 'radix_tree_exceptional_entry'; did you mean 'radix_tree_exception'? [-Werror=implicit-function-declaration]
       WARN_ON_ONCE(!radix_tree_exceptional_entry(entry))) {
                     ^
include/asm-generic/bug.h:69:25: note: in definition of macro 'WARN_ON_ONCE'
  int __ret_warn_on = !!(condition);   \
                         ^~~~~~~~~
fs/dax.c:395:15: error: implicit declaration of function 'slot_locked'; did you mean 'iget_locked'? [-Werror=implicit-function-declaration]
   } else if (!slot_locked(mapping, slot)) {
               ^~~~~~~~~~~
               iget_locked
fs/dax.c:396:4: error: implicit declaration of function 'lock_slot'; did you mean 'local_set'? [-Werror=implicit-function-declaration]
    lock_slot(mapping, slot);
    ^~~~~~~~~
    local_set
fs/dax.c:402:28: error: passing argument 1 of 'dax_entry_waitqueue' from incompatible pointer type [-Werror=incompatible-pointer-types]
   wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
                            ^~~~~~~
fs/dax.c:152:27: note: expected 'struct xa_state *' but argument is of type 'struct address_space *'
 static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
                           ^~~~~~~~~~~~~~~~~~~
fs/dax.c:402:37: warning: passing argument 2 of 'dax_entry_waitqueue' makes pointer from integer without a cast [-Wint-conversion]
   wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
                                     ^~~~~
fs/dax.c:152:27: note: expected 'void *' but argument is of type 'long unsigned int'
 static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
                           ^~~~~~~~~~~~~~~~~~~
fs/dax.c:402:8: error: too many arguments to function 'dax_entry_waitqueue'
   wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
        ^~~~~~~~~~~~~~~~~~~
fs/dax.c:152:27: note: declared here
 static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas,
                           ^~~~~~~~~~~~~~~~~~~
fs/dax.c: In function 'dax_unlock_page':
fs/dax.c:424:2: error: implicit declaration of function 'dax_unlock_mapping_entry'; did you mean 'dax_delete_mapping_entry'? [-Werror=implicit-function-declaration]
  dax_unlock_mapping_entry(mapping, page->index);
  ^~~~~~~~~~~~~~~~~~~~~~~~
  dax_delete_mapping_entry

Caused by commits in the xarray tree interacting with commit

  9b3d53936caa ("filesystem-dax: Introduce dax_lock_page()")

from the nvdimm tree.

Willy thanks for the heads up about this.

I have applied the following merge fix patch (taken from the diff between
the -next tree at this point and the xarray-20180615 branch from the
xarray tree) for today.

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Mon, 18 Jun 2018 13:17:15 +1000
Subject: [PATCH] filesystem-dax: fixups for xaarray changes

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/dax.c | 75 +++++++++++++++++++++++++-------------------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 579088945add..91f7bce6ce64 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -350,78 +350,71 @@ static struct page *dax_busy_page(void *entry)
 
 struct page *dax_lock_page(unsigned long pfn)
 {
-	pgoff_t index;
-	struct inode *inode;
-	wait_queue_head_t *wq;
-	void *entry = NULL, **slot;
+	struct page *page = pfn_to_page(pfn);
+	XA_STATE(xas, NULL, 0);
+	void *entry;
 	struct address_space *mapping;
-	struct wait_exceptional_entry_queue ewait;
-	struct page *ret = NULL, *page = pfn_to_page(pfn);
 
-	rcu_read_lock();
 	for (;;) {
+		rcu_read_lock();
 		mapping = READ_ONCE(page->mapping);
 
-		if (!mapping || !IS_DAX(mapping->host))
+		if (!mapping || !IS_DAX(mapping->host)) {
+			page = NULL;
 			break;
+		}
 
 		/*
 		 * In the device-dax case there's no need to lock, a
 		 * struct dev_pagemap pin is sufficient to keep the
 		 * inode alive.
 		 */
-		inode = mapping->host;
-		if (S_ISCHR(inode->i_mode)) {
-			ret = page;
+		if (S_ISCHR(mapping->host->i_mode))
 			break;
-		}
 
-		xa_lock_irq(&mapping->i_pages);
+		xas.xa = &mapping->i_pages;
+		xas_lock_irq(&xas);
+		rcu_read_unlock();
 		if (mapping != page->mapping) {
 			xa_unlock_irq(&mapping->i_pages);
 			continue;
 		}
-		index = page->index;
-
-		init_wait(&ewait.wait);
-		ewait.wait.func = wake_exceptional_entry_func;
-
-		entry = __radix_tree_lookup(&mapping->i_pages, index, NULL,
-				&slot);
-		if (!entry ||
-		    WARN_ON_ONCE(!radix_tree_exceptional_entry(entry))) {
-			xa_unlock_irq(&mapping->i_pages);
-			break;
-		} else if (!slot_locked(mapping, slot)) {
-			lock_slot(mapping, slot);
-			ret = page;
-			xa_unlock_irq(&mapping->i_pages);
-			break;
+		xas_set(&xas, page->index);
+		entry = xas_load(&xas);
+		if (dax_is_locked(entry)) {
+			entry = get_unlocked_entry(&xas);
+			/* Did the page move while we slept? */
+			if (dax_to_pfn(entry) != pfn) {
+				xas_unlock_irq(&xas);
+				continue;
+			}
 		}
-
-		wq = dax_entry_waitqueue(mapping, index, entry, &ewait.key);
-		prepare_to_wait_exclusive(wq, &ewait.wait,
-				TASK_UNINTERRUPTIBLE);
-		xa_unlock_irq(&mapping->i_pages);
-		rcu_read_unlock();
-		schedule();
-		finish_wait(wq, &ewait.wait);
-		rcu_read_lock();
+		dax_lock_entry(&xas, entry);
+		xas_unlock_irq(&xas);
+		goto out;
 	}
 	rcu_read_unlock();
 
+out:
 	return page;
 }
 
 void dax_unlock_page(struct page *page)
 {
 	struct address_space *mapping = page->mapping;
-	struct inode *inode = mapping->host;
+	XA_STATE(xas, &mapping->i_pages, page->index);
+	void *entry;
 
-	if (S_ISCHR(inode->i_mode))
+	if (S_ISCHR(mapping->host->i_mode))
 		return;
 
-	dax_unlock_mapping_entry(mapping, page->index);
+	xas_lock_irq(&xas);
+	entry = xas_load(&xas);
+	BUG_ON(!dax_is_locked(entry));
+	entry = dax_make_page_entry(page, entry);
+	xas_store(&xas, entry);
+	dax_wake_entry(&xas, entry, false);
+	xas_unlock_irq(&xas);
 }
 
 /*
-- 
2.17.1

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]