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

[Alexander Duyck]

On Thu, Oct 12, 2017 at 8:59 AM, Jesus Sanchez-Palencia
<jesus.sanchez-palencia at intel.com> wrote:
> Hi Alex,
>
>
> On 10/12/2017 08:21 AM, Alexander Duyck wrote:
>> On Wed, Oct 11, 2017 at 5:54 PM, Vinicius Costa Gomes
>> <vinicius.gomes at intel.com> wrote:
>>> From: Jesus Sanchez-Palencia <jesus.sanchez-palencia at intel.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 at intel.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.
>
>
> This was brought up by Cong Wang during the review of our v2. Based on my
> understanding, the point I've made is that for mqprio the inner qdiscs are
> always 'related' to one of the Tx netdev_queues per design. Returning any other
> queue as a fallback seemed like going against that to me.
>
> I'm still inclined to say that we should keep this function as the patch is
> proposing, thus either returning the correct netdev_queue for a given handle, or
> NULL as a way to flag that something was 'wrong' with it. Returning queue #0 is
> misleading in that sense, imho.
>
> As for aligning mq_select_queue() with this approach, if my reasoning behind
> mqprio is correct and also applies to mq, I would be happy to send that fix as
> part our v7.
>
> What do you think?
>
>
> Thanks,
> Jesus

I think it would be better to bring mq_select_queue in line with your
fix. You could probably just add it to your first patch. That way if
the user specifies a bad qdisc classid they don't get to just
overwrite the qdisc on Tx queue 0.

- Alex