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

[Cong Wang <xiyou.wangcong@gmail.com>]

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.