[Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Cong Wang]

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);
 }
 
 static struct tc_action_ops act_mirred_ops = {
-- 
2.13.0

[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 <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

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 <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);
>  }
>  

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 <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.

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Jiri Pirko]

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
>

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Cong Wang]

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;
                }

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Jiri Pirko]

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;
>                }

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Jiri Pirko]

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
>

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Cong Wang]

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.

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Cong Wang]

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.

Re: [Patch net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev()

[Jiri Pirko]

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.