netdev - Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement select_queue class_ops

Open Source and information security mailing list archives

Message-ID: <CAKgT0Ufo1tG4GpSkGaWuUxqhzEUOT7wcBLuaZx_mZmEL+9SnXw@mail.gmail.com>
Date:   Thu, 12 Oct 2017 08:21:29 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Vinicius Costa Gomes <vinicius.gomes@...el.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        rodney.cummings@...com, andre.guedes@...el.com,
        Jiri Pirko <jiri@...nulli.us>, ivan.briano@...el.com,
        Richard Cochran <richardcochran@...il.com>, henrik@...tad.us,
        Jamal Hadi Salim <jhs@...atatu.com>, levipearson@...il.com,
        boon.leong.ong@...el.com, Cong Wang <xiyou.wangcong@...il.com>,
        Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
Subject: Re: [Intel-wired-lan] [next-queue PATCH v6 2/5] mqprio: Implement
 select_queue class_ops

On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
<vinicius.gomes@...el.com> wrote:
> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
>
> When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
> the netdev_queue pointer that the current child qdisc is associated
> with before creating the new qdisc.
>
> Currently, when using mqprio as root qdisc, the kernel will end up
> getting the queue #0 pointer from the mqprio (root qdisc), which leaves
> any new child qdisc with a possibly wrong netdev_queue pointer.
>
> Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
> issue and avoid an inconsistent state when child qdiscs are replaced.
>
> Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@...el.com>
> ---
>  net/sched/sch_mqprio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
> index 6bcdfe6e7b63..8c042ae323e3 100644
> --- a/net/sched/sch_mqprio.c
> +++ b/net/sched/sch_mqprio.c
> @@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>         }
>  }
>
> +static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
> +                                               struct tcmsg *tcm)
> +{
> +       return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
> +}
> +

So I was just comparing this against mq_selet_queue, and I was
wondering why we are willing to return NULL here instead of just
returning a pointer to the first Tx queue? I realize there is the fix
in the first patch but it seems like if we are going to go that route
then maybe we should update mq as well so that both of these qdiscs
behave the same way. Either this should work like mq, or mq should
work like this, but we shouldn't have them exposing different
behaviors.

>  static const struct Qdisc_class_ops mqprio_class_ops = {
>         .graft          = mqprio_graft,
>         .leaf           = mqprio_leaf,
> @@ -403,6 +409,7 @@ static const struct Qdisc_class_ops mqprio_class_ops = {
>         .walk           = mqprio_walk,
>         .dump           = mqprio_dump_class,
>         .dump_stats     = mqprio_dump_class_stats,
> +       .select_queue   = mqprio_select_queue,
>  };
>
>  static struct Qdisc_ops mqprio_qdisc_ops __read_mostly = {
> --
> 2.14.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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