netdev - Re: [PATCH net-next 1/3] net: sched: update default qdisc visibility after Tx queue cnt changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpW553fnxXxC+BLkhzsGixoufNrjzRTrhFKo_gsE9xPwbQ@mail.gmail.com>
Date:   Thu, 16 Sep 2021 22:46:42 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Eric Dumazet <edumazet@...gle.com>,
        Matthew Massey <matthewmassey@...com>,
        Dave Taht <dave.taht@...il.com>
Subject: Re: [PATCH net-next 1/3] net: sched: update default qdisc visibility
 after Tx queue cnt changes

On Wed, Sep 15, 2021 at 12:36 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 15 Sep 2021 09:31:08 -0700 Cong Wang wrote:
> > On Mon, Sep 13, 2021 at 3:53 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 74fd402d26dd..f930329f0dc2 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -2921,6 +2921,8 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
> > >                 if (dev->num_tc)
> > >                         netif_setup_tc(dev, txq);
> > >
> > > +               dev_qdisc_change_real_num_tx(dev, txq);
> > > +
> >
> > Don't we need to flip the device with dev_deactivate()+dev_activate()?
> > It looks like the only thing this function resets is qdisc itself, and only
> > partially.
>
> We're only making the qdiscs visible, there should be
> no datapath-visible change.

Isn't every qdisc under mq visible to datapath?

Packets can be pending in qdisc's, and qdisc's can be scheduled
in TX softirq, so essentially we need to flip the device like other
places.

>
> > >                 dev->real_num_tx_queues = txq;
> > >
> > >                 if (disabling) {
>
> > > diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
> > > index e79f1afe0cfd..db18d8a860f9 100644
> > > --- a/net/sched/sch_mq.c
> > > +++ b/net/sched/sch_mq.c
> > > @@ -125,6 +125,29 @@ static void mq_attach(struct Qdisc *sch)
> > >         priv->qdiscs = NULL;
> > >  }
> > >
> > > +static void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
> >
> > This is nearly identical to mqprio_change_real_num_tx(), can we reuse
> > it?
>
> Indeed, I was a little unsure where best to place the helper.
> Since mq is always built if mqprio is my instinct would be to
> export mq_change_real_num_tx and use it in mqprio. But I didn't
> see any existing exports (mq_attach(), mq_queue_get() are also
> identical and are not shared) so I just copy&pasted the logic.

What about net/sched/sch_generic.c?

>
> LMK if (a) that's fine; (b) I should share the new code;
> (c) I should post a patch to share all the code that's identical;...

I think you can put the code in net/sched/sch_generic.c and export
it for mqprio (mq is built-in so can just call it).

Thanks.