On Thu, Oct 12, 2017 at 8:59 AM, Jesus Sanchez-Palencia <jesus.sanchez-palen...@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.go...@intel.com> wrote: >>> From: Jesus Sanchez-Palencia <jesus.sanchez-palen...@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-palen...@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