Re: [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock

[Cong Wang]

On Thu, Nov 30, 2017 at 3:12 PM, Eric Dumazet  wrote:
> On Thu, 2017-11-30 at 14:53 -0800, Cong Wang wrote:
>> @@ -55,13 +54,10 @@ static void tcf_mirred_release(struct tc_action
>> *a, int bind)
>>   struct tcf_mirred *m = to_mirred(a);
>>   struct net_device *dev;
>>
>> - /* We could be called either in a RCU callback or with RTNL
>> lock held. */
>> - spin_lock_bh(_list_lock);
>>   list_del(>tcfm_list);
>>   dev = rcu_dereference_protected(m->tcfm_dev, 1);
>
> If RTNL is held at this point, I suggest to use
> rtnl_dereference() instead of rcu_dereference_protected() to get proper
> lockdep coverage.

Ah, sure, I missed it. Will send v2 later.

Re: [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock

[Eric Dumazet]

On Thu, 2017-11-30 at 14:53 -0800, Cong Wang wrote:
> TC actions are no longer freed in RCU callbacks and we should
> always have RTNL lock, so this spinlock is no longer needed.
> 
> Cc: Jiri Pirko 
> Cc: Jamal Hadi Salim 
> Signed-off-by: Cong Wang 
> ---
>  net/sched/act_mirred.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index fe6489f9c3cf..2c51952bf2d4 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -29,7 +29,6 @@
>  #include 
>  
>  static LIST_HEAD(mirred_list);
> -static DEFINE_SPINLOCK(mirred_list_lock);
>  
>  static bool tcf_mirred_is_act_redirect(int action)
>  {
> @@ -55,13 +54,10 @@ static void tcf_mirred_release(struct tc_action
> *a, int bind)
>   struct tcf_mirred *m = to_mirred(a);
>   struct net_device *dev;
>  
> - /* We could be called either in a RCU callback or with RTNL
> lock held. */
> - spin_lock_bh(_list_lock);
>   list_del(>tcfm_list);
>   dev = rcu_dereference_protected(m->tcfm_dev, 1);

If RTNL is held at this point, I suggest to use
rtnl_dereference() instead of rcu_dereference_protected() to get proper
lockdep coverage.


>   if (dev)
>   dev_put(dev);
> - spin_unlock_bh(_list_lock);
>  }
>  

[Patch net-next] act_mirred: get rid of mirred_list_lock spinlock

[Cong Wang]

TC actions are no longer freed in RCU callbacks and we should
always have RTNL lock, so this spinlock is no longer needed.

Cc: Jiri Pirko 
Cc: Jamal Hadi Salim 
Signed-off-by: Cong Wang 
---
 net/sched/act_mirred.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fe6489f9c3cf..2c51952bf2d4 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -29,7 +29,6 @@
 #include 
 
 static LIST_HEAD(mirred_list);
-static DEFINE_SPINLOCK(mirred_list_lock);
 
 static bool tcf_mirred_is_act_redirect(int action)
 {
@@ -55,13 +54,10 @@ static void tcf_mirred_release(struct tc_action *a, int 
bind)
struct tcf_mirred *m = to_mirred(a);
struct net_device *dev;
 
-   /* We could be called either in a RCU callback or with RTNL lock held. 
*/
-   spin_lock_bh(_list_lock);
list_del(>tcfm_list);
dev = rcu_dereference_protected(m->tcfm_dev, 1);
if (dev)
dev_put(dev);
-   spin_unlock_bh(_list_lock);
 }
 
 static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
@@ -148,9 +144,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr 
*nla,
}
 
if (ret == ACT_P_CREATED) {
-   spin_lock_bh(_list_lock);
list_add(>tcfm_list, _list);
-   spin_unlock_bh(_list_lock);
tcf_idr_insert(tn, *a);
}
 
@@ -293,7 +287,6 @@ static int mirred_device_event(struct notifier_block 
*unused,
 
ASSERT_RTNL();
if (event == NETDEV_UNREGISTER) {
-   spin_lock_bh(_list_lock);
list_for_each_entry(m, _list, tcfm_list) {
if (rcu_access_pointer(m->tcfm_dev) == dev) {
dev_put(dev);
@@ -303,7 +296,6 @@ static int mirred_device_event(struct notifier_block 
*unused,
RCU_INIT_POINTER(m->tcfm_dev, NULL);
}
}
-   spin_unlock_bh(_list_lock);
}
 
return NOTIFY_DONE;
-- 
2.13.0