[nf-next] net: netfilter: nf_tables_api: Use id allocation.

Message ID 20180216184918.9781-1-rvarsha016@gmail.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [nf-next] net: netfilter: nf_tables_api: Use id allocation.
Related show

Commit Message

Varsha Rao Feb. 16, 2018, 6:49 p.m.
In nf_tables_set_alloc_name function, remove get_zeroed_page
find_first_zero_bit and set_bit functions. Instead use ida_simple_get
function as it simplifies the code.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
 net/netfilter/nf_tables_api.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

Comments

Pablo Neira Ayuso March 11, 2018, 9:03 p.m. | #1
Hi Varsha,

On Sat, Feb 17, 2018 at 12:19:18AM +0530, Varsha Rao wrote:
> In nf_tables_set_alloc_name function, remove get_zeroed_page
> find_first_zero_bit and set_bit functions. Instead use ida_simple_get
> function as it simplifies the code.
> 
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
>  net/netfilter/nf_tables_api.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0791813a1e7d..08fbb5ffab3a 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2653,18 +2653,14 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
>  {
>  	const struct nft_set *i;
>  	const char *p;
> -	unsigned long *inuse;
> -	unsigned int n = 0, min = 0;
> +	int n = 0;
> +	DEFINE_IDA(inuse);
>  
>  	p = strchr(name, '%');
>  	if (p != NULL) {
>  		if (p[1] != 'd' || strchr(p + 2, '%'))
>  			return -EINVAL;
>  
> -		inuse = (unsigned long *)get_zeroed_page(GFP_KERNEL);
> -		if (inuse == NULL)
> -			return -ENOMEM;
> -cont:
>  		list_for_each_entry(i, &ctx->table->sets, list) {
>  			int tmp;
>  
> @@ -2672,22 +2668,21 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
>  				continue;
>  			if (!sscanf(i->name, name, &tmp))
>  				continue;
> -			if (tmp < min || tmp >= min + BITS_PER_BYTE * PAGE_SIZE)
> +			if (tmp < 0 || tmp >= BITS_PER_BYTE * PAGE_SIZE)
>  				continue;
>  
> -			set_bit(tmp - min, inuse);
> +			n = ida_simple_get(&inuse, tmp, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
> +			if (n < 0)
> +				return n;
>  		}
> +		n = ida_simple_get(&inuse, 0, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
> +		if (n < 0)
> +			return n;
>  
> -		n = find_first_zero_bit(inuse, BITS_PER_BYTE * PAGE_SIZE);
> -		if (n >= BITS_PER_BYTE * PAGE_SIZE) {
> -			min += BITS_PER_BYTE * PAGE_SIZE;
> -			memset(inuse, 0, PAGE_SIZE);
> -			goto cont;
> -		}
> -		free_page((unsigned long)inuse);
> +		ida_destroy(&inuse);

I think after this patch, we end up having an upper limit of PAGE_SIZE
sets. This limit was not there before.

BTW, could we just do a full conversion to ida? I mean, we could store
the struct ida in struct nft_table, so we don't need to rebuild this
bitmap everytime we need to allocate a set.

It will consume more memory but that's reasonable.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Varsha Rao March 13, 2018, 7:35 p.m. | #2
Hello Pablo,

On Mon, Mar 12, 2018 at 2:33 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Varsha,
>
> On Sat, Feb 17, 2018 at 12:19:18AM +0530, Varsha Rao wrote:
> > In nf_tables_set_alloc_name function, remove get_zeroed_page
> > find_first_zero_bit and set_bit functions. Instead use ida_simple_get
> > function as it simplifies the code.
> >
> > Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> > ---
> >  net/netfilter/nf_tables_api.c | 27 +++++++++++----------------
> >  1 file changed, 11 insertions(+), 16 deletions(-)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 0791813a1e7d..08fbb5ffab3a 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -2653,18 +2653,14 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
> >  {
> >       const struct nft_set *i;
> >       const char *p;
> > -     unsigned long *inuse;
> > -     unsigned int n = 0, min = 0;
> > +     int n = 0;
> > +     DEFINE_IDA(inuse);
> >
> >       p = strchr(name, '%');
> >       if (p != NULL) {
> >               if (p[1] != 'd' || strchr(p + 2, '%'))
> >                       return -EINVAL;
> >
> > -             inuse = (unsigned long *)get_zeroed_page(GFP_KERNEL);
> > -             if (inuse == NULL)
> > -                     return -ENOMEM;
> > -cont:
> >               list_for_each_entry(i, &ctx->table->sets, list) {
> >                       int tmp;
> >
> > @@ -2672,22 +2668,21 @@ static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
> >                               continue;
> >                       if (!sscanf(i->name, name, &tmp))
> >                               continue;
> > -                     if (tmp < min || tmp >= min + BITS_PER_BYTE * PAGE_SIZE)
> > +                     if (tmp < 0 || tmp >= BITS_PER_BYTE * PAGE_SIZE)
> >                               continue;
> >
> > -                     set_bit(tmp - min, inuse);
> > +                     n = ida_simple_get(&inuse, tmp, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
> > +                     if (n < 0)
> > +                             return n;
> >               }
> > +             n = ida_simple_get(&inuse, 0, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
> > +             if (n < 0)
> > +                     return n;
> >
> > -             n = find_first_zero_bit(inuse, BITS_PER_BYTE * PAGE_SIZE);
> > -             if (n >= BITS_PER_BYTE * PAGE_SIZE) {
> > -                     min += BITS_PER_BYTE * PAGE_SIZE;
> > -                     memset(inuse, 0, PAGE_SIZE);
> > -                     goto cont;
> > -             }
> > -             free_page((unsigned long)inuse);
> > +             ida_destroy(&inuse);
>
> I think after this patch, we end up having an upper limit of PAGE_SIZE
> sets. This limit was not there before.
>
I will change it back.

>
> BTW, could we just do a full conversion to ida? I mean, we could store
> the struct ida in struct nft_table, so we don't need to rebuild this
> bitmap everytime we need to allocate a set.
>
> It will consume more memory but that's reasonable.

I think it is better to construct rather than add ida to struct nf_table.
As we are destroying the structure after getting the required id. It is
okay to allocate id while traversing the list and after completion of list
traversal.

Thanks,
Varsha
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 0791813a1e7d..08fbb5ffab3a 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2653,18 +2653,14 @@  static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 {
 	const struct nft_set *i;
 	const char *p;
-	unsigned long *inuse;
-	unsigned int n = 0, min = 0;
+	int n = 0;
+	DEFINE_IDA(inuse);
 
 	p = strchr(name, '%');
 	if (p != NULL) {
 		if (p[1] != 'd' || strchr(p + 2, '%'))
 			return -EINVAL;
 
-		inuse = (unsigned long *)get_zeroed_page(GFP_KERNEL);
-		if (inuse == NULL)
-			return -ENOMEM;
-cont:
 		list_for_each_entry(i, &ctx->table->sets, list) {
 			int tmp;
 
@@ -2672,22 +2668,21 @@  static int nf_tables_set_alloc_name(struct nft_ctx *ctx, struct nft_set *set,
 				continue;
 			if (!sscanf(i->name, name, &tmp))
 				continue;
-			if (tmp < min || tmp >= min + BITS_PER_BYTE * PAGE_SIZE)
+			if (tmp < 0 || tmp >= BITS_PER_BYTE * PAGE_SIZE)
 				continue;
 
-			set_bit(tmp - min, inuse);
+			n = ida_simple_get(&inuse, tmp, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
+			if (n < 0)
+				return n;
 		}
+		n = ida_simple_get(&inuse, 0, BITS_PER_BYTE * PAGE_SIZE, GFP_KERNEL);
+		if (n < 0)
+			return n;
 
-		n = find_first_zero_bit(inuse, BITS_PER_BYTE * PAGE_SIZE);
-		if (n >= BITS_PER_BYTE * PAGE_SIZE) {
-			min += BITS_PER_BYTE * PAGE_SIZE;
-			memset(inuse, 0, PAGE_SIZE);
-			goto cont;
-		}
-		free_page((unsigned long)inuse);
+		ida_destroy(&inuse);
 	}
 
-	set->name = kasprintf(GFP_KERNEL, name, min + n);
+	set->name = kasprintf(GFP_KERNEL, name, n);
 	if (!set->name)
 		return -ENOMEM;