| Message ID | 20171130225335.6957-1-xiyou.wangcong@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] act_mirred: use tcfm_dev in tcf_mirred_get_dev() | expand |
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 >
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; }
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; > }
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 >
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.
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.
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.
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 = {
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(-)