From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michel Machado Subject: Re: [PATCH net-next] net:sched: add gkprio scheduler Date: Wed, 9 May 2018 10:09:40 -0400 Message-ID: References: <20180507093626.GA5794@gmail.com> <20180508101210.GB4383@gmail.com> <273f91db-7a2f-acda-b306-5a78dd948478@digirati.com.br> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Nishanth Devarajan , Jiri Pirko , Jamal Hadi Salim , David Miller , Linux Kernel Network Developers , Cody Doucette To: Cong Wang Return-path: Received: from mta113.f1.k8.com.br ([187.73.32.185]:56115 "EHLO mta113.f1.k8.com.br" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756452AbeEIOJw (ORCPT ); Wed, 9 May 2018 10:09:52 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 05/08/2018 10:24 PM, Cong Wang wrote: > On Tue, May 8, 2018 at 5:59 AM, Michel Machado wrote: >>>> Overall it looks good to me, just one thing below: >>>> >>>>> +struct Qdisc_ops gkprio_qdisc_ops __read_mostly = { >>>>> + .id = "gkprio", >>>>> + .priv_size = sizeof(struct gkprio_sched_data), >>>>> + .enqueue = gkprio_enqueue, >>>>> + .dequeue = gkprio_dequeue, >>>>> + .peek = qdisc_peek_dequeued, >>>>> + .init = gkprio_init, >>>>> + .reset = gkprio_reset, >>>>> + .change = gkprio_change, >>>>> + .dump = gkprio_dump, >>>>> + .destroy = gkprio_destroy, >>>>> + .owner = THIS_MODULE, >>>>> +}; >>>> >>>> >>>> You probably want to add Qdisc_class_ops here so that you can >>>> dump the stats of each internal queue. >> >> >> Hi Cong, >> >> In the production scenario we are targeting, this priority queue must be >> classless; being classful would only bloat the code for us. I don't see >> making this queue classful as a problem per se, but I suggest leaving it as >> a future improvement for when someone can come up with a useful scenario for >> it. > > > Take a look at sch_prio, it is fairly simple since your internal > queues are just an array... Per-queue stats are quite useful > in production, we definitely want to observe which queues are > full which are not. > DSprio cannot add Qdisc_class_ops without a rewrite of other queue disciplines, which doesn't seem desirable. Since the method cops->leaf is required (see register_qdisc()), we would need to replace the array struct sk_buff_head qdiscs[GKPRIO_MAX_PRIORITY] in struct gkprio_sched_data with the array struct Qdisc *queues[GKPRIO_MAX_PRIORITY] to be able to return a Qdisc in dsprio_leaf(). The problem with this change is that Qdisc does not have a method to dequeue from its tail. This new method may not even make sense in other queue disciplines. But without this method, gkprio_enqueue() cannot drop the lowest priority packet when the queue is full and an incoming packet has higher priority. Nevertheless, I see your point on being able to observe the distribution of queued packets per priority. A solution for that would be to add the array __u32 qlen[GKPRIO_MAX_PRIORITY] in struct tc_gkprio_qopt. This solution even avoids adding overhead in the critical paths of DSprio. Do you see a better solution? By the way, I've used GKPRIO_MAX_PRIORITY and other names that include "gkprio" above to reflect the version 1 of this patch that we are discussing. We will rename these identifiers for version 2 of this patch to replace "gkprio" with "dsprio". [ ]'s Michel Machado