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

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

[Stephen Rothwell]

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

Hi Matthew,

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

net/netfilter/nf_tables_api.c: In function 'nf_tables_set_alloc_name':
net/netfilter/nf_tables_api.c:3014:8: error: implicit declaration of function 'ida_get_new_above'; did you mean 'idr_get_next_ul'? [-Werror=implicit-function-declaration]
    n = ida_get_new_above(&inuse, tmp, &id);
        ^~~~~~~~~~~~~~~~~
        idr_get_next_ul

Caused by commit

  3f2668c1e101 ("ida: Remove old API")

interacting with commit

  9679150a0bd5 ("netfilter: nf_tables: Use id allocation")

from the netfilter-next tree.

I took a guess and applied the following merge fix patch.

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Wed, 18 Jul 2018 16:42:26 +1000
Subject: [PATCH] ida: merge fix for ida_get_new_above() removal

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 net/netfilter/nf_tables_api.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index b7b5fbcda8dd..151b89174979 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2995,7 +2995,7 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 {
 	const struct nft_set *i;
 	const char *p;
-	unsigned int n = 0, id = 0;
+	int id = 0;
 	DEFINE_IDA(inuse);
 
 	p = strchr(name, '%');
@@ -3011,22 +3011,22 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 			if (!sscanf(i->name, name, &tmp))
 				continue;
 
-			n = ida_get_new_above(&inuse, tmp, &id);
-			if (n < 0) {
-				if (n == -EAGAIN)
+			id = ida_alloc_min(&inuse, tmp, GFP_KERNEL);
+			if (id < 0) {
+				if (id == -EAGAIN)
 					return -ENOMEM;
 
-				return n;
+				return id;
 			}
 		}
 
-		n = ida_get_new_above(&inuse, 0, &id);
+		id = ida_alloc(&inuse, GFP_KERNEL);
 		ida_destroy(&inuse);
 
-		if (n < 0) {
-			if (n == -EAGAIN)
+		if (id < 0) {
+			if (id == -EAGAIN)
 				return -ENOMEM;
-			return n;
+			return id;
 		}
 
 	}

-- 
Cheers,
Stephen Rothwell

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

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

[Pablo Neira Ayuso]

Hi Matthew, Stephen,

On Wed, Jul 18, 2018 at 04:54:06PM +1000, Stephen Rothwell wrote:
> Hi Matthew,
> 
> After merging the ida tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
> 
> net/netfilter/nf_tables_api.c: In function 'nf_tables_set_alloc_name':
> net/netfilter/nf_tables_api.c:3014:8: error: implicit declaration of function 'ida_get_new_above'; did you mean 'idr_get_next_ul'? [-Werror=implicit-function-declaration]
>     n = ida_get_new_above(&inuse, tmp, &id);
>         ^~~~~~~~~~~~~~~~~
>         idr_get_next_ul
> 
> Caused by commit
> 
>   3f2668c1e101 ("ida: Remove old API")

I see, we have no more lockless API for IDA anymore :-(. In our case,
we were already protected by the the nfnl_lock mutex, which it was
sufficient to ensure non-concurrent access to IDA structures.

Unless I'm missing anything, the new API forces use to the spinlock
call with disabled irq for each time we update something from the
netfilter netlink interface, so that's a no-go for us.

> interacting with commit
> 
>   9679150a0bd5 ("netfilter: nf_tables: Use id allocation")
> 
> from the netfilter-next tree.

@Varsha, I'm very sorry, but I guess I have to toss your patch, I
would prefer avoid dependencies with the IDA API by now.

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

[Matthew Wilcox]

On Wed, Jul 18, 2018 at 11:24:26AM +0200, Pablo Neira Ayuso wrote:
> I see, we have no more lockless API for IDA anymore :-(. In our case,
> we were already protected by the the nfnl_lock mutex, which it was
> sufficient to ensure non-concurrent access to IDA structures.

You're actually the first user for whom this is true.  For every other
user, the requirement to manage their own spinlock was a pain.

> Unless I'm missing anything, the new API forces use to the spinlock
> call with disabled irq for each time we update something from the
> netfilter netlink interface, so that's a no-go for us.

I can't believe that's a serious problem for you, though.  You're calling
sscanf(), this can't possibly be a performance path.

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

[Pablo Neira Ayuso]

On Wed, Jul 18, 2018 at 04:59:19AM -0700, Matthew Wilcox wrote:
> On Wed, Jul 18, 2018 at 11:24:26AM +0200, Pablo Neira Ayuso wrote:
> > I see, we have no more lockless API for IDA anymore :-(. In our case,
> > we were already protected by the the nfnl_lock mutex, which it was
> > sufficient to ensure non-concurrent access to IDA structures.
> 
> You're actually the first user for whom this is true.  For every other
> user, the requirement to manage their own spinlock was a pain.
> 
> > Unless I'm missing anything, the new API forces use to the spinlock
> > call with disabled irq for each time we update something from the
> > netfilter netlink interface, so that's a no-go for us.
> 
> I can't believe that's a serious problem for you, though.  You're calling
> sscanf(), this can't possibly be a performance path.

It's not about performance, this is control plane code. This is
disabling irqs, which is something we don't need.

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

[Matthew Wilcox]

On Wed, Jul 18, 2018 at 11:24:26AM +0200, Pablo Neira Ayuso wrote:
> > interacting with commit
> > 
> >   9679150a0bd5 ("netfilter: nf_tables: Use id allocation")
> > 
> > from the netfilter-next tree.
> 
> @Varsha, I'm very sorry, but I guess I have to toss your patch, I
> would prefer avoid dependencies with the IDA API by now.

I've had a chance to read this patch a bit more carefully.  It transforms
one anti-pattern into another, so I can't say I'm a fan.

The first is specific to the networking code; having a list of things
with IDs, and constructing a bitmap when we need to allocate a new ID.

The second is having both an IDA and a list of things.

The more effective way to do all of this is to use an IDR.  You can get
rid of the linked list *and* the IDA, and it's faster to iterate over.
The root of the IDR is the same size as the list_head, and then you need
only store the 4-byte ID in each element instead of the 16-byte list_head.

So Varsha, if you would like to take a look at transforming table->sets
from a LIST_HEAD to an IDR, I think that would be a great use of your
time.

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

[Pablo Neira Ayuso]

On Wed, Jul 18, 2018 at 06:14:46AM -0700, Matthew Wilcox wrote:
> On Wed, Jul 18, 2018 at 11:24:26AM +0200, Pablo Neira Ayuso wrote:
> > > interacting with commit
> > > 
> > >   9679150a0bd5 ("netfilter: nf_tables: Use id allocation")
> > > 
> > > from the netfilter-next tree.
> > 
> > @Varsha, I'm very sorry, but I guess I have to toss your patch, I
> > would prefer avoid dependencies with the IDA API by now.
> 
> I've had a chance to read this patch a bit more carefully.  It transforms
> one anti-pattern into another, so I can't say I'm a fan.
> 
> The first is specific to the networking code; having a list of things
> with IDs, and constructing a bitmap when we need to allocate a new ID.
> 
> The second is having both an IDA and a list of things.
> 
> The more effective way to do all of this is to use an IDR.  You can get
> rid of the linked list *and* the IDA, and it's faster to iterate over.
> The root of the IDR is the same size as the list_head, and then you need
> only store the 4-byte ID in each element instead of the 16-byte list_head.
> 
> So Varsha, if you would like to take a look at transforming table->sets
> from a LIST_HEAD to an IDR, I think that would be a great use of your
> time.

Please, don't do so, we don't need a radix tree datastructure, it's
just more complexity.

We just wanted to have a simple way to allocate IDs using a bitmap
structure in the background without reinventing the wheel.

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

[Matthew Wilcox]

On Wed, Jul 18, 2018 at 03:27:46PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 18, 2018 at 06:14:46AM -0700, Matthew Wilcox wrote:
> > So Varsha, if you would like to take a look at transforming table->sets
> > from a LIST_HEAD to an IDR, I think that would be a great use of your
> > time.
> 
> Please, don't do so, we don't need a radix tree datastructure, it's
> just more complexity.

It's no more complex to use than the list_* macros.

> We just wanted to have a simple way to allocate IDs using a bitmap
> structure in the background without reinventing the wheel.

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

[Pablo Neira Ayuso]

On Wed, Jul 18, 2018 at 06:31:26AM -0700, Matthew Wilcox wrote:
> On Wed, Jul 18, 2018 at 03:27:46PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, Jul 18, 2018 at 06:14:46AM -0700, Matthew Wilcox wrote:
> > > So Varsha, if you would like to take a look at transforming table->sets
> > > from a LIST_HEAD to an IDR, I think that would be a great use of your
> > > time.
> > 
> > Please, don't do so, we don't need a radix tree datastructure, it's
> > just more complexity.
> 
> It's no more complex to use than the list_* macros.

Problem is that some of the sets that we place in that list may have
no ID.

We basically have two type of sets:

* Sets with names, they have no IDs as the user provides a meaningful
  name from the control plane that can be used to add/delete elements,
  eg. IP addresses.

* Anonymous sets, these are built-in into rules, eg.

  ip saddr { 1.1.1.1, 2.2.2.2 }

  so we generate an ID that we can use to refer to the set.

For our usecase, I'm thinking, if we don't have a simple way to
allocate IDs through this API, we could just simplify our existing
codebase by using an u64 and use incremental id, we don't need to
recycle IDs, so that's one posibility I stop bothering you ;-)

BTW, the anti-pattern we have in our codebase is the same logic that we
have to allocate identifiers with netdevice name, see __dev_alloc_name()
in net/core/dev.c. *Someone* copied + pasted + mangled that original code
to make it fit into netfilter. I guess that code may benefit from a
simple way to allocate IDs without locking dependencies. Just an idea,
not that this is a priority.

Thanks!