diff mbox series

[net-next,v10,03/14] net: sched: wrap doit/dumpit methods

Message ID 20250302000901.2729164-4-sdf@fomichev.me (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: Hold netdev instance lock during ndo operations | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success Found: 'dev_put(' was: 1 now: 1
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 277 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Stanislav Fomichev March 2, 2025, 12:08 a.m. UTC 
In preparation for grabbing netdev instance lock around qdisc
operations, introduce tc_xxx wrappers that lookup netdev
and call respective __tc_xxx helper to do the actual work.
No functional changes.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
 1 file changed, 122 insertions(+), 68 deletions(-)

Comments

Jamal Hadi Salim March 2, 2025, 8:49 p.m. UTC | #1 
On Sat, Mar 1, 2025 at 7:09 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> In preparation for grabbing netdev instance lock around qdisc
> operations, introduce tc_xxx wrappers that lookup netdev
> and call respective __tc_xxx helper to do the actual work.
> No functional changes.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: Saeed Mahameed <saeed@kernel.org>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
>  1 file changed, 122 insertions(+), 68 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index e3e91cf867eb..e0be3af4daa9 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1505,27 +1505,18 @@ const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
>   * Delete/get qdisc.
>   */
>
[..]
> +static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> +                          struct netlink_ext_ack *extack)
> +{
> +       struct net *net = sock_net(skb->sk);
> +       struct tcmsg *tcm = nlmsg_data(n);
> +       struct nlattr *tca[TCA_MAX + 1];
> +       struct net_device *dev;
> +       bool replay;
> +       int err;
> +
> +replay:

For 1-1 mapping to original code, the line:
struct tcmsg *tcm = nlmsg_data(n);

Should move below the replay goto..

Other than that:
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

> +       /* Reinit, just in case something touches this. */
> +       err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
> +                                    rtm_tca_policy, extack);

[..]

cheers,
jamal
Stanislav Fomichev March 3, 2025, 3:33 p.m. UTC | #2 
On 03/02, Jamal Hadi Salim wrote:
> On Sat, Mar 1, 2025 at 7:09 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > In preparation for grabbing netdev instance lock around qdisc
> > operations, introduce tc_xxx wrappers that lookup netdev
> > and call respective __tc_xxx helper to do the actual work.
> > No functional changes.
> >
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: Cong Wang <xiyou.wangcong@gmail.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>
> > Cc: Saeed Mahameed <saeed@kernel.org>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
> >  1 file changed, 122 insertions(+), 68 deletions(-)
> >
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index e3e91cf867eb..e0be3af4daa9 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -1505,27 +1505,18 @@ const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> >   * Delete/get qdisc.
> >   */
> >
> [..]
> > +static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> > +                          struct netlink_ext_ack *extack)
> > +{
> > +       struct net *net = sock_net(skb->sk);
> > +       struct tcmsg *tcm = nlmsg_data(n);
> > +       struct nlattr *tca[TCA_MAX + 1];
> > +       struct net_device *dev;
> > +       bool replay;
> > +       int err;
> > +
> > +replay:
> 
> For 1-1 mapping to original code, the line:
> struct tcmsg *tcm = nlmsg_data(n);
> 
> Should move below the replay goto..

Since nlmsg_data is just doing pointer arithmetics and the (pointer) argument
doesn't change I was assuming it's ok to reshuffle to make
tc_get_qdisc and tc_modify_qdisc look more consistent. LMK if you
disagree, happy to move them back.


> Other than that:
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

Thank you for the review!
Jamal Hadi Salim March 4, 2025, 4:43 p.m. UTC | #3 
On Mon, Mar 3, 2025 at 10:33 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 03/02, Jamal Hadi Salim wrote:
> > On Sat, Mar 1, 2025 at 7:09 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > In preparation for grabbing netdev instance lock around qdisc
> > > operations, introduce tc_xxx wrappers that lookup netdev
> > > and call respective __tc_xxx helper to do the actual work.
> > > No functional changes.
> > >
> > > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > > Cc: Cong Wang <xiyou.wangcong@gmail.com>
> > > Cc: Jiri Pirko <jiri@resnulli.us>
> > > Cc: Saeed Mahameed <saeed@kernel.org>
> > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > ---
> > >  net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
> > >  1 file changed, 122 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > index e3e91cf867eb..e0be3af4daa9 100644
> > > --- a/net/sched/sch_api.c
> > > +++ b/net/sched/sch_api.c
> > > @@ -1505,27 +1505,18 @@ const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> > >   * Delete/get qdisc.
> > >   */
> > >
> > [..]
> > > +static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> > > +                          struct netlink_ext_ack *extack)
> > > +{
> > > +       struct net *net = sock_net(skb->sk);
> > > +       struct tcmsg *tcm = nlmsg_data(n);
> > > +       struct nlattr *tca[TCA_MAX + 1];
> > > +       struct net_device *dev;
> > > +       bool replay;
> > > +       int err;
> > > +
> > > +replay:
> >
> > For 1-1 mapping to original code, the line:
> > struct tcmsg *tcm = nlmsg_data(n);
> >
> > Should move below the replay goto..
>
> Since nlmsg_data is just doing pointer arithmetics and the (pointer) argument
> doesn't change I was assuming it's ok to reshuffle to make
> tc_get_qdisc and tc_modify_qdisc look more consistent. LMK if you
> disagree, happy to move them back.
>

TBH, I never understood why we had to reinit for the netlink messaging
per that comment: /* Reinit, just in case something touches this. */
I dont think anything will change that netlink message, but who knows
there could be some niche case.
IOW, if you are going to respin for another reason then do it for
equivalence sake.
Worst case, we may be able finally find out why the code is that way
when someone reports a strange bug ;->

cheers,
jamal

>
> > Other than that:
> > Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Thank you for the review!
Cong Wang March 4, 2025, 8:53 p.m. UTC | #4 
On Tue, Mar 04, 2025 at 11:43:17AM -0500, Jamal Hadi Salim wrote:
> On Mon, Mar 3, 2025 at 10:33 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 03/02, Jamal Hadi Salim wrote:
> > > On Sat, Mar 1, 2025 at 7:09 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > In preparation for grabbing netdev instance lock around qdisc
> > > > operations, introduce tc_xxx wrappers that lookup netdev
> > > > and call respective __tc_xxx helper to do the actual work.
> > > > No functional changes.
> > > >
> > > > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > Cc: Cong Wang <xiyou.wangcong@gmail.com>
> > > > Cc: Jiri Pirko <jiri@resnulli.us>
> > > > Cc: Saeed Mahameed <saeed@kernel.org>
> > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > > ---
> > > >  net/sched/sch_api.c | 190 ++++++++++++++++++++++++++++----------------
> > > >  1 file changed, 122 insertions(+), 68 deletions(-)
> > > >
> > > > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > > > index e3e91cf867eb..e0be3af4daa9 100644
> > > > --- a/net/sched/sch_api.c
> > > > +++ b/net/sched/sch_api.c
> > > > @@ -1505,27 +1505,18 @@ const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
> > > >   * Delete/get qdisc.
> > > >   */
> > > >
> > > [..]
> > > > +static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> > > > +                          struct netlink_ext_ack *extack)
> > > > +{
> > > > +       struct net *net = sock_net(skb->sk);
> > > > +       struct tcmsg *tcm = nlmsg_data(n);
> > > > +       struct nlattr *tca[TCA_MAX + 1];
> > > > +       struct net_device *dev;
> > > > +       bool replay;
> > > > +       int err;
> > > > +
> > > > +replay:
> > >
> > > For 1-1 mapping to original code, the line:
> > > struct tcmsg *tcm = nlmsg_data(n);
> > >
> > > Should move below the replay goto..
> >
> > Since nlmsg_data is just doing pointer arithmetics and the (pointer) argument
> > doesn't change I was assuming it's ok to reshuffle to make
> > tc_get_qdisc and tc_modify_qdisc look more consistent. LMK if you
> > disagree, happy to move them back.
> >
> 
> TBH, I never understood why we had to reinit for the netlink messaging
> per that comment: /* Reinit, just in case something touches this. */
> I dont think anything will change that netlink message, but who knows
> there could be some niche case.

As long as all TDC tests pass, we can keep the patch as it is. Now you
can see why I have been pushing people to add TDC tests, they are a
warranty of safety for code refactoring like this one. :)

Thanks.
diff mbox series

Patch

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e3e91cf867eb..e0be3af4daa9 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1505,27 +1505,18 @@  const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
  * Delete/get qdisc.
  */
 
-static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
-			struct netlink_ext_ack *extack)
+static int __tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
+			  struct netlink_ext_ack *extack,
+			  struct net_device *dev,
+			  struct nlattr *tca[TCA_MAX + 1],
+			  struct tcmsg *tcm)
 {
 	struct net *net = sock_net(skb->sk);
-	struct tcmsg *tcm = nlmsg_data(n);
-	struct nlattr *tca[TCA_MAX + 1];
-	struct net_device *dev;
-	u32 clid;
 	struct Qdisc *q = NULL;
 	struct Qdisc *p = NULL;
+	u32 clid;
 	int err;
 
-	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
-				     rtm_tca_policy, extack);
-	if (err < 0)
-		return err;
-
-	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
-		return -ENODEV;
-
 	clid = tcm->tcm_parent;
 	if (clid) {
 		if (clid != TC_H_ROOT) {
@@ -1582,6 +1573,27 @@  static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	return 0;
 }
 
+static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
+			struct netlink_ext_ack *extack)
+{
+	struct net *net = sock_net(skb->sk);
+	struct tcmsg *tcm = nlmsg_data(n);
+	struct nlattr *tca[TCA_MAX + 1];
+	struct net_device *dev;
+	int err;
+
+	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
+				     rtm_tca_policy, extack);
+	if (err < 0)
+		return err;
+
+	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	return __tc_get_qdisc(skb, n, extack, dev, tca, tcm);
+}
+
 static bool req_create_or_replace(struct nlmsghdr *n)
 {
 	return (n->nlmsg_flags & NLM_F_CREATE &&
@@ -1601,35 +1613,19 @@  static bool req_change(struct nlmsghdr *n)
 		!(n->nlmsg_flags & NLM_F_EXCL));
 }
 
-/*
- * Create/change qdisc.
- */
-static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
-			   struct netlink_ext_ack *extack)
+static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
+			     struct netlink_ext_ack *extack,
+			     struct net_device *dev,
+			     struct nlattr *tca[TCA_MAX + 1],
+			     struct tcmsg *tcm,
+			     bool *replay)
 {
-	struct net *net = sock_net(skb->sk);
-	struct tcmsg *tcm;
-	struct nlattr *tca[TCA_MAX + 1];
-	struct net_device *dev;
+	struct Qdisc *q = NULL;
+	struct Qdisc *p = NULL;
 	u32 clid;
-	struct Qdisc *q, *p;
 	int err;
 
-replay:
-	/* Reinit, just in case something touches this. */
-	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
-				     rtm_tca_policy, extack);
-	if (err < 0)
-		return err;
-
-	tcm = nlmsg_data(n);
 	clid = tcm->tcm_parent;
-	q = p = NULL;
-
-	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
-		return -ENODEV;
-
 
 	if (clid) {
 		if (clid != TC_H_ROOT) {
@@ -1755,7 +1751,7 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	}
 	err = qdisc_change(q, tca, extack);
 	if (err == 0)
-		qdisc_notify(net, skb, n, clid, NULL, q, extack);
+		qdisc_notify(sock_net(skb->sk), skb, n, clid, NULL, q, extack);
 	return err;
 
 create_n_graft:
@@ -1788,8 +1784,10 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 				 tca, &err, extack);
 	}
 	if (q == NULL) {
-		if (err == -EAGAIN)
-			goto replay;
+		if (err == -EAGAIN) {
+			*replay = true;
+			return 0;
+		}
 		return err;
 	}
 
@@ -1804,6 +1802,38 @@  static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
 	return 0;
 }
 
+/*
+ * Create/change qdisc.
+ */
+static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
+			   struct netlink_ext_ack *extack)
+{
+	struct net *net = sock_net(skb->sk);
+	struct tcmsg *tcm = nlmsg_data(n);
+	struct nlattr *tca[TCA_MAX + 1];
+	struct net_device *dev;
+	bool replay;
+	int err;
+
+replay:
+	/* Reinit, just in case something touches this. */
+	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
+				     rtm_tca_policy, extack);
+	if (err < 0)
+		return err;
+
+	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	replay = false;
+	err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm, &replay);
+	if (replay)
+		goto replay;
+
+	return err;
+}
+
 static int tc_dump_qdisc_root(struct Qdisc *root, struct sk_buff *skb,
 			      struct netlink_callback *cb,
 			      int *q_idx_p, int s_q_idx, bool recur,
@@ -2135,15 +2165,15 @@  static void tc_bind_tclass(struct Qdisc *q, u32 portid, u32 clid,
 
 #endif
 
-static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
-			 struct netlink_ext_ack *extack)
+static int __tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
+			   struct netlink_ext_ack *extack,
+			   struct net_device *dev,
+			   struct nlattr *tca[TCA_MAX + 1],
+			   struct tcmsg *tcm)
 {
 	struct net *net = sock_net(skb->sk);
-	struct tcmsg *tcm = nlmsg_data(n);
-	struct nlattr *tca[TCA_MAX + 1];
-	struct net_device *dev;
-	struct Qdisc *q = NULL;
 	const struct Qdisc_class_ops *cops;
+	struct Qdisc *q = NULL;
 	unsigned long cl = 0;
 	unsigned long new_cl;
 	u32 portid;
@@ -2151,15 +2181,6 @@  static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 	u32 qid;
 	int err;
 
-	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
-				     rtm_tca_policy, extack);
-	if (err < 0)
-		return err;
-
-	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
-		return -ENODEV;
-
 	/*
 	   parent == TC_H_UNSPEC - unspecified parent.
 	   parent == TC_H_ROOT   - class is root, which has no parent.
@@ -2268,6 +2289,27 @@  static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
 	return err;
 }
 
+static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n,
+			 struct netlink_ext_ack *extack)
+{
+	struct net *net = sock_net(skb->sk);
+	struct tcmsg *tcm = nlmsg_data(n);
+	struct nlattr *tca[TCA_MAX + 1];
+	struct net_device *dev;
+	int err;
+
+	err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
+				     rtm_tca_policy, extack);
+	if (err < 0)
+		return err;
+
+	dev = __dev_get_by_index(net, tcm->tcm_ifindex);
+	if (!dev)
+		return -ENODEV;
+
+	return __tc_ctl_tclass(skb, n, extack, dev, tca, tcm);
+}
+
 struct qdisc_dump_args {
 	struct qdisc_walker	w;
 	struct sk_buff		*skb;
@@ -2344,20 +2386,12 @@  static int tc_dump_tclass_root(struct Qdisc *root, struct sk_buff *skb,
 	return 0;
 }
 
-static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb)
+static int __tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb,
+			    struct tcmsg *tcm, struct net_device *dev)
 {
-	struct tcmsg *tcm = nlmsg_data(cb->nlh);
-	struct net *net = sock_net(skb->sk);
 	struct netdev_queue *dev_queue;
-	struct net_device *dev;
 	int t, s_t;
 
-	if (nlmsg_len(cb->nlh) < sizeof(*tcm))
-		return 0;
-	dev = dev_get_by_index(net, tcm->tcm_ifindex);
-	if (!dev)
-		return 0;
-
 	s_t = cb->args[0];
 	t = 0;
 
@@ -2374,10 +2408,30 @@  static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb)
 done:
 	cb->args[0] = t;
 
-	dev_put(dev);
 	return skb->len;
 }
 
+static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb)
+{
+	struct tcmsg *tcm = nlmsg_data(cb->nlh);
+	struct net *net = sock_net(skb->sk);
+	struct net_device *dev;
+	int err;
+
+	if (nlmsg_len(cb->nlh) < sizeof(*tcm))
+		return 0;
+
+	dev = dev_get_by_index(net, tcm->tcm_ifindex);
+	if (!dev)
+		return 0;
+
+	err = __tc_dump_tclass(skb, cb, tcm, dev);
+
+	dev_put(dev);
+
+	return err;
+}
+
 #ifdef CONFIG_PROC_FS
 static int psched_show(struct seq_file *seq, void *v)
 {