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_iQpVec_FY-71n3VUUgo8YCcn00+QzBBck9h1RGNaFzXX_ig@mail.gmail.com>
Date:   Wed, 15 Sep 2021 09:31:08 -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 Mon, Sep 13, 2021 at 3:53 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> mq / mqprio make the default child qdiscs visible. They only do
> so for the qdiscs which are within real_num_tx_queues when the
> device is registered. Depending on order of calls in the driver,
> or if user space changes config via ethtool -L the number of
> qdiscs visible under tc qdisc show will differ from the number
> of queues. This is confusing to users and potentially to system
> configuration scripts which try to make sure qdiscs have the
> right parameters.
>
> Add a new Qdisc_ops callback and make relevant qdiscs TTRT.
>
> Note that this uncovers the "shortcut" created by
> commit 1f27cde313d7 ("net: sched: use pfifo_fast for non real queues")
> The default child qdiscs beyond initial real_num_tx are always
> pfifo_fast, no matter what the sysfs setting is. Fixing this
> gets a little tricky because we'd need to keep a reference
> on whatever the default qdisc was at the time of creation.
> In practice this is likely an non-issue the qdiscs likely have
> to be configured to non-default settings, so whatever user space
> is doing such configuration can replace the pfifos... now that
> it will see them.
>

Looks reasonable.

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


>                 dev->real_num_tx_queues = txq;
>
>                 if (disabling) {
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a8dd06c74e31..66d2fbe9ef50 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1330,6 +1330,15 @@ static int qdisc_change_tx_queue_len(struct net_device *dev,
>         return 0;
>  }
>
> +void dev_qdisc_change_real_num_tx(struct net_device *dev,
> +                                 unsigned int new_real_tx)
> +{
> +       struct Qdisc *qdisc = dev->qdisc;
> +
> +       if (qdisc->ops->change_real_num_tx)
> +               qdisc->ops->change_real_num_tx(qdisc, new_real_tx);
> +}
> +
>  int dev_qdisc_change_tx_queue_len(struct net_device *dev)
>  {
>         bool up = dev->flags & IFF_UP;
> 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?

Thanks.