Re: [PATCH v3 10/11] net: sched: atomically check-allocate action
On Mon, May 28, 2018 at 12:17:28AM +0300, Vlad Buslov wrote:
> Implement function that atomically checks if action exists and either takes
> reference to it, or allocates idr slot for action index to prevent
> concurrent allocations of actions with same index. Use EBUSY error pointer
> to indicate that idr slot is reserved.
>
> Implement cleanup helper function that removes temporary error pointer from
> idr. (in case of error between idr allocation and insertion of newly
> created action to specified index)
>
> Refactor all action init functions to insert new action to idr using this
> API.
>
> Signed-off-by: Vlad Buslov
Reviewed-by: Marcelo Ricardo Leitner
> ---
> Changes from V1 to V2:
> - Remove unique idr insertion function. Change original idr insert to do
> the same thing.
> - Refactor action check-alloc code into standalone function.
>
> include/net/act_api.h | 3 ++
> net/sched/act_api.c| 92
> --
> net/sched/act_bpf.c| 11 --
> net/sched/act_connmark.c | 10 +++--
> net/sched/act_csum.c | 11 --
> net/sched/act_gact.c | 11 --
> net/sched/act_ife.c| 6 ++-
> net/sched/act_ipt.c| 13 ++-
> net/sched/act_mirred.c | 16 ++--
> net/sched/act_nat.c| 11 --
> net/sched/act_pedit.c | 15 ++--
> net/sched/act_police.c | 9 -
> net/sched/act_sample.c | 11 --
> net/sched/act_simple.c | 11 +-
> net/sched/act_skbedit.c| 11 +-
> net/sched/act_skbmod.c | 11 +-
> net/sched/act_tunnel_key.c | 9 -
> net/sched/act_vlan.c | 17 -
> 18 files changed, 218 insertions(+), 60 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index d256e20507b9..cd4547476074 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -154,6 +154,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
> struct nlattr *est,
> int bind, bool cpustats);
> void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
>
> +void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
> +int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
> + struct tc_action **a, int bind);
> int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
> int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index eefe8c2fe667..9511502e1cbb 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -303,7 +303,9 @@ static bool __tcf_idr_check(struct tc_action_net *tn, u32
> index,
>
> spin_lock(&idrinfo->lock);
> p = idr_find(&idrinfo->action_idr, index);
> - if (p) {
> + if (IS_ERR(p)) {
> + p = NULL;
> + } else if (p) {
> refcount_inc(&p->tcfa_refcnt);
> if (bind)
> atomic_inc(&p->tcfa_bindcnt);
> @@ -371,7 +373,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
> struct nlattr *est,
> {
> struct tc_action *p = kzalloc(ops->size, GFP_KERNEL);
> struct tcf_idrinfo *idrinfo = tn->idrinfo;
> - struct idr *idr = &idrinfo->action_idr;
> int err = -ENOMEM;
>
> if (unlikely(!p))
> @@ -389,20 +390,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
> struct nlattr *est,
> goto err2;
> }
> spin_lock_init(&p->tcfa_lock);
> - idr_preload(GFP_KERNEL);
> - spin_lock(&idrinfo->lock);
> - /* user doesn't specify an index */
> - if (!index) {
> - index = 1;
> - err = idr_alloc_u32(idr, NULL, &index, UINT_MAX, GFP_ATOMIC);
> - } else {
> - err = idr_alloc_u32(idr, NULL, &index, index, GFP_ATOMIC);
> - }
> - spin_unlock(&idrinfo->lock);
> - idr_preload_end();
> - if (err)
> - goto err3;
> -
> p->tcfa_index = index;
> p->tcfa_tm.install = jiffies;
> p->tcfa_tm.lastuse = jiffies;
> @@ -412,7 +399,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
> struct nlattr *est,
> &p->tcfa_rate_est,
> &p->tcfa_lock, NULL, est);
> if (err)
> - goto err4;
> + goto err3;
> }
>
> p->idrinfo = idrinfo;
> @@ -420,8 +407,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
> struct nlattr *est,
> INIT_LIST_HEAD(&p->list);
> *a = p;
> return 0;
> -err4:
> - idr_remove(idr, index);
> err3:
> free_percpu(p->cpu_qstats);
> err2:
> @@ -437,11 +422,78 @@ void tcf_idr_insert(struct tc_action_net *tn, struct
> tc_action *a)
> struct tcf_idrinfo *idrinfo = tn->idrinfo;
>
> spin_lock(&idrinfo->lock);
> - idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
> + /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc *
Re: [PATCH v3 10/11] net: sched: atomically check-allocate action
Sun, May 27, 2018 at 11:17:28PM CEST, vla...@mellanox.com wrote: >Implement function that atomically checks if action exists and either takes >reference to it, or allocates idr slot for action index to prevent >concurrent allocations of actions with same index. Use EBUSY error pointer >to indicate that idr slot is reserved. > >Implement cleanup helper function that removes temporary error pointer from >idr. (in case of error between idr allocation and insertion of newly >created action to specified index) > >Refactor all action init functions to insert new action to idr using this >API. > >Signed-off-by: Vlad Buslov Signed-off-by: Jiri Pirko
[PATCH v3 10/11] net: sched: atomically check-allocate action
Implement function that atomically checks if action exists and either takes
reference to it, or allocates idr slot for action index to prevent
concurrent allocations of actions with same index. Use EBUSY error pointer
to indicate that idr slot is reserved.
Implement cleanup helper function that removes temporary error pointer from
idr. (in case of error between idr allocation and insertion of newly
created action to specified index)
Refactor all action init functions to insert new action to idr using this
API.
Signed-off-by: Vlad Buslov
---
Changes from V1 to V2:
- Remove unique idr insertion function. Change original idr insert to do
the same thing.
- Refactor action check-alloc code into standalone function.
include/net/act_api.h | 3 ++
net/sched/act_api.c| 92 --
net/sched/act_bpf.c| 11 --
net/sched/act_connmark.c | 10 +++--
net/sched/act_csum.c | 11 --
net/sched/act_gact.c | 11 --
net/sched/act_ife.c| 6 ++-
net/sched/act_ipt.c| 13 ++-
net/sched/act_mirred.c | 16 ++--
net/sched/act_nat.c| 11 --
net/sched/act_pedit.c | 15 ++--
net/sched/act_police.c | 9 -
net/sched/act_sample.c | 11 --
net/sched/act_simple.c | 11 +-
net/sched/act_skbedit.c| 11 +-
net/sched/act_skbmod.c | 11 +-
net/sched/act_tunnel_key.c | 9 -
net/sched/act_vlan.c | 17 -
18 files changed, 218 insertions(+), 60 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index d256e20507b9..cd4547476074 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -154,6 +154,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
struct nlattr *est,
int bind, bool cpustats);
void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
+void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
+int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
+ struct tc_action **a, int bind);
int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index eefe8c2fe667..9511502e1cbb 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -303,7 +303,9 @@ static bool __tcf_idr_check(struct tc_action_net *tn, u32
index,
spin_lock(&idrinfo->lock);
p = idr_find(&idrinfo->action_idr, index);
- if (p) {
+ if (IS_ERR(p)) {
+ p = NULL;
+ } else if (p) {
refcount_inc(&p->tcfa_refcnt);
if (bind)
atomic_inc(&p->tcfa_bindcnt);
@@ -371,7 +373,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
struct nlattr *est,
{
struct tc_action *p = kzalloc(ops->size, GFP_KERNEL);
struct tcf_idrinfo *idrinfo = tn->idrinfo;
- struct idr *idr = &idrinfo->action_idr;
int err = -ENOMEM;
if (unlikely(!p))
@@ -389,20 +390,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
struct nlattr *est,
goto err2;
}
spin_lock_init(&p->tcfa_lock);
- idr_preload(GFP_KERNEL);
- spin_lock(&idrinfo->lock);
- /* user doesn't specify an index */
- if (!index) {
- index = 1;
- err = idr_alloc_u32(idr, NULL, &index, UINT_MAX, GFP_ATOMIC);
- } else {
- err = idr_alloc_u32(idr, NULL, &index, index, GFP_ATOMIC);
- }
- spin_unlock(&idrinfo->lock);
- idr_preload_end();
- if (err)
- goto err3;
-
p->tcfa_index = index;
p->tcfa_tm.install = jiffies;
p->tcfa_tm.lastuse = jiffies;
@@ -412,7 +399,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
struct nlattr *est,
&p->tcfa_rate_est,
&p->tcfa_lock, NULL, est);
if (err)
- goto err4;
+ goto err3;
}
p->idrinfo = idrinfo;
@@ -420,8 +407,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index,
struct nlattr *est,
INIT_LIST_HEAD(&p->list);
*a = p;
return 0;
-err4:
- idr_remove(idr, index);
err3:
free_percpu(p->cpu_qstats);
err2:
@@ -437,11 +422,78 @@ void tcf_idr_insert(struct tc_action_net *tn, struct
tc_action *a)
struct tcf_idrinfo *idrinfo = tn->idrinfo;
spin_lock(&idrinfo->lock);
- idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
+ /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
+ WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
spin_unlock(&idrinfo->lock);
}
EXPORT_SYMBOL(tcf_idr_insert);
+/* Cleanup idr index that was allocated but not initialized. */
+
+void tcf_idr_cleanup(struct tc_acti
