| 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. | expand |
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
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
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;
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(-)