* [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock
2017-11-30 22:53 [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Cong Wang
@ 2017-11-30 22:53 ` Cong Wang
2017-11-30 23:12 ` Eric Dumazet
2017-12-01 17:56 ` [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Jiri Pirko
2017-12-02 8:57 ` Jiri Pirko
2 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-11-30 22:53 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jiri Pirko, Jamal Hadi Salim
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 <jiri@mellanox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
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 <net/tc_act/tc_mirred.h>
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(&mirred_list_lock);
list_del(&m->tcfm_list);
dev = rcu_dereference_protected(m->tcfm_dev, 1);
if (dev)
dev_put(dev);
- spin_unlock_bh(&mirred_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(&mirred_list_lock);
list_add(&m->tcfm_list, &mirred_list);
- spin_unlock_bh(&mirred_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(&mirred_list_lock);
list_for_each_entry(m, &mirred_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(&mirred_list_lock);
}
return NOTIFY_DONE;
--
2.13.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock
2017-11-30 22:53 ` [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock Cong Wang
@ 2017-11-30 23:12 ` Eric Dumazet
2017-12-01 1:02 ` Cong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Eric Dumazet @ 2017-11-30 23:12 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: Jiri Pirko, Jamal Hadi Salim
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 <jiri@mellanox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> 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 <net/tc_act/tc_mirred.h>
>
> 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(&mirred_list_lock);
> list_del(&m->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(&mirred_list_lock);
> }
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock
2017-11-30 23:12 ` Eric Dumazet
@ 2017-12-01 1:02 ` Cong Wang
0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2017-12-01 1:02 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
On Thu, Nov 30, 2017 at 3:12 PM, Eric Dumazet <eric.dumazet@gmail.com> 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(&mirred_list_lock);
>> list_del(&m->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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-11-30 22:53 [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Cong Wang
2017-11-30 22:53 ` [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock Cong Wang
@ 2017-12-01 17:56 ` Jiri Pirko
2017-12-01 21:46 ` Cong Wang
2017-12-02 8:57 ` Jiri Pirko
2 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2017-12-01 17:56 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Jiri Pirko, Jamal Hadi Salim
Thu, Nov 30, 2017 at 11:53:32PM CET, xiyou.wangcong@gmail.com wrote:
>tcfm_dev always points to the correct netdev and we already
>hold a refcnt, so no need to use ifindex to lookup again.
>
>If we would support moving target netdev across netns, using
>pointer would be better than ifindex.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> include/net/tc_act/tc_mirred.h | 1 -
> net/sched/act_mirred.c | 3 +--
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>index 21d253c9a8c6..b2dbbfaefd22 100644
>--- a/include/net/tc_act/tc_mirred.h
>+++ b/include/net/tc_act/tc_mirred.h
>@@ -11,7 +11,6 @@ struct tcf_mirred {
> int tcfm_ifindex;
> bool tcfm_mac_header_xmit;
> struct net_device __rcu *tcfm_dev;
>- struct net *net;
> struct list_head tcfm_list;
> };
> #define to_mirred(a) ((struct tcf_mirred *)a)
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 8b3e59388480..fe6489f9c3cf 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -140,7 +140,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> m->tcfm_eaction = parm->eaction;
> if (dev != NULL) {
> m->tcfm_ifindex = parm->ifindex;
>- m->net = net;
Isn't this here so user may specify a ifindex of netdev which is not yet
present on the system (not sure how much sense that would make though...)
> if (ret != ACT_P_CREATED)
> dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
> dev_hold(dev);
>@@ -318,7 +317,7 @@ static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
> {
> struct tcf_mirred *m = to_mirred(a);
>
>- return __dev_get_by_index(m->net, m->tcfm_ifindex);
>+ return rtnl_dereference(m->tcfm_dev);
> }
>
> static struct tc_action_ops act_mirred_ops = {
>--
>2.13.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-12-01 17:56 ` [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Jiri Pirko
@ 2017-12-01 21:46 ` Cong Wang
2017-12-02 6:56 ` Jiri Pirko
0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-12-01 21:46 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
On Fri, Dec 1, 2017 at 9:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>
> Isn't this here so user may specify a ifindex of netdev which is not yet
> present on the system (not sure how much sense that would make though...)
How is this even possible? If an ifindex is not present, we return ENODEV:
if (parm->ifindex) {
dev = __dev_get_by_index(net, parm->ifindex);
if (dev == NULL) {
if (exists)
tcf_idr_release(*a, bind);
return -ENODEV;
}
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-12-01 21:46 ` Cong Wang
@ 2017-12-02 6:56 ` Jiri Pirko
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2017-12-02 6:56 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
Fri, Dec 01, 2017 at 10:46:42PM CET, xiyou.wangcong@gmail.com wrote:
>On Fri, Dec 1, 2017 at 9:56 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Isn't this here so user may specify a ifindex of netdev which is not yet
>> present on the system (not sure how much sense that would make though...)
>
>How is this even possible? If an ifindex is not present, we return ENODEV:
Right, I missed this. Thanks.
>
> if (parm->ifindex) {
> dev = __dev_get_by_index(net, parm->ifindex);
> if (dev == NULL) {
> if (exists)
> tcf_idr_release(*a, bind);
> return -ENODEV;
> }
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-11-30 22:53 [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Cong Wang
2017-11-30 22:53 ` [Patch net-next] act_mirred: get rid of mirred_list_lock spinlock Cong Wang
2017-12-01 17:56 ` [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() Jiri Pirko
@ 2017-12-02 8:57 ` Jiri Pirko
2017-12-02 19:47 ` Cong Wang
2 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2017-12-02 8:57 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Jiri Pirko, Jamal Hadi Salim
Thu, Nov 30, 2017 at 11:53:32PM CET, xiyou.wangcong@gmail.com wrote:
>tcfm_dev always points to the correct netdev and we already
>hold a refcnt, so no need to use ifindex to lookup again.
>
>If we would support moving target netdev across netns, using
>pointer would be better than ifindex.
>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>---
> include/net/tc_act/tc_mirred.h | 1 -
> net/sched/act_mirred.c | 3 +--
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>index 21d253c9a8c6..b2dbbfaefd22 100644
>--- a/include/net/tc_act/tc_mirred.h
>+++ b/include/net/tc_act/tc_mirred.h
>@@ -11,7 +11,6 @@ struct tcf_mirred {
> int tcfm_ifindex;
> bool tcfm_mac_header_xmit;
> struct net_device __rcu *tcfm_dev;
>- struct net *net;
> struct list_head tcfm_list;
> };
> #define to_mirred(a) ((struct tcf_mirred *)a)
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 8b3e59388480..fe6489f9c3cf 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -140,7 +140,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> m->tcfm_eaction = parm->eaction;
> if (dev != NULL) {
> m->tcfm_ifindex = parm->ifindex;
>- m->net = net;
> if (ret != ACT_P_CREATED)
> dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
> dev_hold(dev);
>@@ -318,7 +317,7 @@ static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
> {
> struct tcf_mirred *m = to_mirred(a);
>
>- return __dev_get_by_index(m->net, m->tcfm_ifindex);
>+ return rtnl_dereference(m->tcfm_dev);
Good. Please also use m->tcfm_dev->ifindex in tcf_mirred_dump and
tcf_mirred_ifindex. Then you can remove tcfm_ifindex completely.
> }
>
> static struct tc_action_ops act_mirred_ops = {
>--
>2.13.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-12-02 8:57 ` Jiri Pirko
@ 2017-12-02 19:47 ` Cong Wang
2017-12-02 19:53 ` Cong Wang
0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-12-02 19:47 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
On Sat, Dec 2, 2017 at 12:57 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Good. Please also use m->tcfm_dev->ifindex in tcf_mirred_dump and
> tcf_mirred_ifindex. Then you can remove tcfm_ifindex completely.
Sounds good. Will send v2.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-12-02 19:47 ` Cong Wang
@ 2017-12-02 19:53 ` Cong Wang
2017-12-03 6:39 ` Jiri Pirko
0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-12-02 19:53 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
On Sat, Dec 2, 2017 at 11:47 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Sat, Dec 2, 2017 at 12:57 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Good. Please also use m->tcfm_dev->ifindex in tcf_mirred_dump and
>> tcf_mirred_ifindex. Then you can remove tcfm_ifindex completely.
>
> Sounds good. Will send v2.
Hold on, m->tcfm_dev could be NULL, so we can't just def it here.
I think we can just use 0 when it is NULL:
.ifindex = m->tcfm_dev ? m->tcfm_dev->ifindex : 0,
This also "fixes" the garbage ifindex dump when the target device is gone.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()
2017-12-02 19:53 ` Cong Wang
@ 2017-12-03 6:39 ` Jiri Pirko
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2017-12-03 6:39 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Jiri Pirko, Jamal Hadi Salim
Sat, Dec 02, 2017 at 08:53:20PM CET, xiyou.wangcong@gmail.com wrote:
>On Sat, Dec 2, 2017 at 11:47 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Sat, Dec 2, 2017 at 12:57 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Good. Please also use m->tcfm_dev->ifindex in tcf_mirred_dump and
>>> tcf_mirred_ifindex. Then you can remove tcfm_ifindex completely.
>>
>> Sounds good. Will send v2.
>
>Hold on, m->tcfm_dev could be NULL, so we can't just def it here.
>
>I think we can just use 0 when it is NULL:
>
>.ifindex = m->tcfm_dev ? m->tcfm_dev->ifindex : 0,
>
>This also "fixes" the garbage ifindex dump when the target device is gone.
Sounds fine.
^ permalink raw reply [flat|nested] 11+ messages in thread