netdev - Re: [PATCH net v1] net: taprio offload: enforce qdisc to netdev queue mapping

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]

Date:   Fri, 14 May 2021 16:47:02 -0700
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Yannick Vignon <yannick.vignon@....nxp.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Joakim Zhang <qiangqing.zhang@....com>,
        sebastien.laveze@....nxp.com,
        Yannick Vignon <yannick.vignon@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>,
        Vladimir Oltean <olteanv@...il.com>,
        Vedang Patel <vedang.patel@...el.com>,
        Michael Walle <michael@...le.cc>
Subject: Re: [PATCH net v1] net: taprio offload: enforce qdisc to netdev
 queue mapping

Jakub Kicinski <kuba@...nel.org> writes:

> On Fri, 14 May 2021 13:40:58 -0700 Vinicius Costa Gomes wrote:
>> Jakub Kicinski <kuba@...nel.org> writes:
>> > You haven't CCed anyone who worked on this Qdisc in the last 2 years :/
>> > CCing them now. Comments, anyone?  
>> 
>> I guess I should suggest myself as maintainer, to reduce chances of this
>> happening again.
>
> Yes, please.
>
>> > This looks like a very drastic change. Are you expecting the qdisc will
>> > always be bypassed?  
>> 
>> Only when running in full offload mode it will be bypassed.
>> 
>> And it's kind of by design, in offload mode, the idea was: configure the
>> netdev traffic class to queue mapping, send the schedule to the hardware
>> and stay out of the way.
>> 
>> But as per Yannick's report, it seems that taprio doesn't stay enough
>> out of the yay.
>> 
>> > After a 1 minute looks it seems like taprio is using device queues in
>> > strict priority fashion. Maybe a different model is needed, but a qdisc
>> > with:
>> >
>> > enqueue()
>> > {
>> > 	WARN_ONCE(1)
>> > }
>> >
>> > really doesn't look right to me.  
>> 
>> This patch takes the "stay out of the way" to the extreme, I kind of
>> like it/I am not opposed to it, if I had this idea a couple of years
>> ago, perhaps I would have used this same approach.
>
> Sorry for my ignorance, but for TXTIME is the hardware capable of
> reordering or the user is supposed to know how to send packets?

At least the hardware that I am familiar with doesn't reorder packets.

For TXTIME, we have ETF (the qdisc) that re-order packets. The way
things work when taprio and ETF are used together is something like
this: taprio only has enough knowledge about TXTIME to drop packets that
would be transmitted outside their "transmission window" (e.g. for
traffic class 0 the transmission window is only for 10 to 50, the TXTIME
for a packet is 60, this packet is "invalid" and is dropped). And then
when the packet is enqueued to the "child" ETF, it's re-ordered and then
sent to the driver.

And this is something that this patch breaks, the ability of dropping
those invalid packets (I really wouldn't like to do this verification
inside our drivers). Thanks for noticing this.

>
> My biggest problem with this patch is that unless the application is
> very careful that WARN_ON_ONCE(1) will trigger. E.g. if softirq is
> servicing the queue when the application sends - the qdisc will not 
> be bypassed, right?
>
>> I am now thinking if this idea locks us out of anything.
>> 
>> Anyway, a nicer alternative would exist if we had a way to tell the core
>> "this qdisc should be bypassed" (i.e. don't call enqueue()/dequeue())
>> after init() runs.
>
> I don't think calling enqueue() and dequeue() is a problem. The problem
> is that RT process does unrelated work.

That is true. But this seems like a much bigger (or at least more
"core") issue.


Cheers,
-- 
Vinicius