From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michel Machado Subject: Re: [PATCH net-next] net:sched: add gkprio scheduler Date: Tue, 8 May 2018 10:56:24 -0400 Message-ID: References: <20180507093626.GA5794@gmail.com> <20180508101210.GB4383@gmail.com> <273f91db-7a2f-acda-b306-5a78dd948478@digirati.com.br> <2855ce67-f1bc-c04c-81ae-70ae3fdc6b17@mojatatu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org, doucette@bu.edu To: Jamal Hadi Salim , Nishanth Devarajan , Cong Wang Return-path: Received: from mta113.f1.k8.com.br ([187.73.32.185]:39009 "EHLO mta113.f1.k8.com.br" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932452AbeEHO4e (ORCPT ); Tue, 8 May 2018 10:56:34 -0400 In-Reply-To: <2855ce67-f1bc-c04c-81ae-70ae3fdc6b17@mojatatu.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 05/08/2018 09:29 AM, Jamal Hadi Salim wrote: > On 08/05/18 08: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. > > I am actually struggling with this whole thing. > Have you considered using skb->prio instead of peeking into the packet > header. > Also have you looked at the dsmark qdisc? As far as I know, skb->priority (skb->prio has been renamed) is unsigned for packets that come from the network. DSprio, adopting Cong's name suggestion, is most useful "merging" packets that come from different network interfaces. Had we relied on DSmark to mark skb->tc_index with the DS field, we would have forced anyone using DSprio to use DSmark. This may sound as a good idea, but DSmark always requires writable socket buffers while setting skb->tc_index with the DS field of the packet (see dsmark_enqueue()), what means that the kernel may drop high priority packets instead of low priority packets due to memory pressure. [ ]'s Michel Machado