netdev - Re: [PATCH net-next] net:sched: add gkprio scheduler

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <efc2b37b-d603-45dd-2776-b9d2750b59bf@digirati.com.br>
Date:   Tue, 8 May 2018 10:56:24 -0400
From:   Michel Machado <michel@...irati.com.br>
To:     Jamal Hadi Salim <jhs@...atatu.com>,
        Nishanth Devarajan <ndev2021@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     jiri@...nulli.us, davem@...emloft.net, netdev@...r.kernel.org,
        doucette@...edu
Subject: Re: [PATCH net-next] net:sched: add gkprio scheduler

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