Re: [PATCH net-next v6 06/11] net: sched: add 'delete' function to action ops

2018-08-10 Thread Vlad Buslov 


On Thu 09 Aug 2018 at 19:38, Cong Wang  wrote:
> On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov  wrote:
>>
>> Extend action ops with 'delete' function. Each action type to implements
>> its own delete function that doesn't depend on rtnl lock.
>>
>> Implement delete function that is required to delete actions without
>> holding rtnl lock. Use action API function that atomically deletes action
>> only if it is still in action idr. This implementation prevents concurrent
>> threads from deleting same action twice.
>
> I fail to understand why you introduce ops->delete(), it seems all
> you want is getting the tn->idrinfo, but you already have tc_action
> before calling ops->delete(), and tc_action has ->idrinfo...
>
> Each type of action does the same too, that is, just calling
> tcf_idr_delete_index()...

I agree with your assessment. Should have implemented it by just calling
tcf_idr_delete_index() directly.

>
> This changelog sucks again, it claims for skipping rtnl lock,
> but you can skip rtnl lock by just calling tcf_idr_delete_index()
> directly too, it is not the reason for adding ops->delete().

My intention was to implement some generic and parallel-safe API that
could be used to implement delete for all actions. It turns out that, as
you noted, just calling tcf_idr_delete_index() is enough because any
special per-action delete code is already implemented by ops->cleanup().

Re: [PATCH net-next v6 06/11] net: sched: add 'delete' function to action ops

2018-08-09 Thread Cong Wang 
On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov  wrote:
>
> Extend action ops with 'delete' function. Each action type to implements
> its own delete function that doesn't depend on rtnl lock.
>
> Implement delete function that is required to delete actions without
> holding rtnl lock. Use action API function that atomically deletes action
> only if it is still in action idr. This implementation prevents concurrent
> threads from deleting same action twice.

I fail to understand why you introduce ops->delete(), it seems all
you want is getting the tn->idrinfo, but you already have tc_action
before calling ops->delete(), and tc_action has ->idrinfo...

Each type of action does the same too, that is, just calling
tcf_idr_delete_index()...

This changelog sucks again, it claims for skipping rtnl lock,
but you can skip rtnl lock by just calling tcf_idr_delete_index()
directly too, it is not the reason for adding ops->delete().

[PATCH net-next v6 06/11] net: sched: add 'delete' function to action ops

2018-07-05 Thread Vlad Buslov 
Extend action ops with 'delete' function. Each action type to implements
its own delete function that doesn't depend on rtnl lock.

Implement delete function that is required to delete actions without
holding rtnl lock. Use action API function that atomically deletes action
only if it is still in action idr. This implementation prevents concurrent
threads from deleting same action twice.

Reviewed-by: Marcelo Ricardo Leitner 
Signed-off-by: Vlad Buslov 
Signed-off-by: Jiri Pirko 
---
Changes from V1 to V2:
- Merge action ops delete definition and implementation.

 include/net/act_api.h  |  1 +
 net/sched/act_bpf.c|  8 
 net/sched/act_connmark.c   |  8 
 net/sched/act_csum.c   |  8 
 net/sched/act_gact.c   |  8 
 net/sched/act_ife.c|  8 
 net/sched/act_ipt.c| 16 
 net/sched/act_mirred.c |  8 
 net/sched/act_nat.c|  8 
 net/sched/act_pedit.c  |  8 
 net/sched/act_police.c |  8 
 net/sched/act_sample.c |  8 
 net/sched/act_simple.c |  8 
 net/sched/act_skbedit.c|  8 
 net/sched/act_skbmod.c |  8 
 net/sched/act_tunnel_key.c |  8 
 net/sched/act_vlan.c   |  8 
 17 files changed, 137 insertions(+)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index a8eaae67c264..b9ed2b8256a5 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -101,6 +101,7 @@ struct tc_action_ops {
void(*stats_update)(struct tc_action *, u64, u32, u64);
size_t  (*get_fill_size)(const struct tc_action *act);
struct net_device *(*get_dev)(const struct tc_action *a);
+   int (*delete)(struct net *net, u32 index);
 };
 
 struct tc_action_net {
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 8ebf40a3506c..7941dd66ff83 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -388,6 +388,13 @@ static int tcf_bpf_search(struct net *net, struct 
tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
 }
 
+static int tcf_bpf_delete(struct net *net, u32 index)
+{
+   struct tc_action_net *tn = net_generic(net, bpf_net_id);
+
+   return tcf_idr_delete_index(tn, index);
+}
+
 static struct tc_action_ops act_bpf_ops __read_mostly = {
.kind   =   "bpf",
.type   =   TCA_ACT_BPF,
@@ -398,6 +405,7 @@ static struct tc_action_ops act_bpf_ops __read_mostly = {
.init   =   tcf_bpf_init,
.walk   =   tcf_bpf_walker,
.lookup =   tcf_bpf_search,
+   .delete =   tcf_bpf_delete,
.size   =   sizeof(struct tcf_bpf),
 };
 
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index e3787aa0025a..143c2d3de723 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -193,6 +193,13 @@ static int tcf_connmark_search(struct net *net, struct 
tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
 }
 
+static int tcf_connmark_delete(struct net *net, u32 index)
+{
+   struct tc_action_net *tn = net_generic(net, connmark_net_id);
+
+   return tcf_idr_delete_index(tn, index);
+}
+
 static struct tc_action_ops act_connmark_ops = {
.kind   =   "connmark",
.type   =   TCA_ACT_CONNMARK,
@@ -202,6 +209,7 @@ static struct tc_action_ops act_connmark_ops = {
.init   =   tcf_connmark_init,
.walk   =   tcf_connmark_walker,
.lookup =   tcf_connmark_search,
+   .delete =   tcf_connmark_delete,
.size   =   sizeof(struct tcf_connmark_info),
 };
 
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 334261943f9f..3768539340e0 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -654,6 +654,13 @@ static size_t tcf_csum_get_fill_size(const struct 
tc_action *act)
return nla_total_size(sizeof(struct tc_csum));
 }
 
+static int tcf_csum_delete(struct net *net, u32 index)
+{
+   struct tc_action_net *tn = net_generic(net, csum_net_id);
+
+   return tcf_idr_delete_index(tn, index);
+}
+
 static struct tc_action_ops act_csum_ops = {
.kind   = "csum",
.type   = TCA_ACT_CSUM,
@@ -665,6 +672,7 @@ static struct tc_action_ops act_csum_ops = {
.walk   = tcf_csum_walker,
.lookup = tcf_csum_search,
.get_fill_size  = tcf_csum_get_fill_size,
+   .delete = tcf_csum_delete,
.size   = sizeof(struct tcf_csum),
 };
 
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index b4dfb2b4addc..a431a711f0dd 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -231,6 +231,13 @@ static size_t tcf_gact_get_fill_size(const struct 
tc_action *act)
return sz;
 }
 
+static int tcf_gact_delete(struct net *net, u32 index)
+{
+   struct