netdev - Re: [PATCH RESEND net-next 0/5] Improve the taprio qdisc's relationship with its children

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: <20230606163156.7ee6uk7jevggmaba@skbuf>
Date: Tue, 6 Jun 2023 19:31:56 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	Vinicius Costa Gomes <vinicius.gomes@...el.com>,
	linux-kernel@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
	Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>,
	Peilin Ye <yepeilin.cs@...il.com>,
	Pedro Tammela <pctammela@...atatu.com>
Subject: Re: [PATCH RESEND net-next 0/5] Improve the taprio qdisc's
 relationship with its children

On Tue, Jun 06, 2023 at 11:39:32AM -0400, Jamal Hadi Salim wrote:
> 1)Just some details become confusing in regards to offload vs not; F.e
> class grafting (taprio_graft()) is de/activating the device but that
> seems only needed for offload. Would it not be better to have those
> separate and call graft_offload vs graft_software, etc? We really need
> to create a generic document on how someone would write code for
> qdiscs for consistency (I started working on one but never completed
> it - if there is a volunteer i would be happy to work with one to
> complete it).

I would be a happy reader of that document. I haven't studied whether
dev_deactivate() and dev_activate() are necessary for the pure software
data path, where the root taprio is also directly attached to the netdev
TXQs and that fact doesn't change across its lifetime.

> 2) It seems like in mqprio this qdisc can only be root qdisc (like
> mqprio)

so far so good

> and you dont want to replace the children with other types of
> qdiscs i.e the children are always pfifo? i.e is it possible or
> intended for example to replace 8001:x with bfifo etc? or even change
> the pfifo queue size, etc?

no, this is not true, why do you say this?

> 3) Offload intention seems really to be bypassing enqueue() and going
> straigth to the driver xmit() for a specific DMA ring that the skb is
> mapped to. Except for the case where the driver says it's busy and
> refuses to stash the skb in ring in which case you have to requeue to
> the appropriate child qdisc/class. I am not sure how that would work
> here - mqprio gets away with it by not defining any of the
> en/de/requeue() callbacks

wait, there is a requeue() callback? where?

> - but likely it will be the lack of requeue that makes it work.

Looking at dev_requeue_skb(), isn't it always going to be requeued to
the same qdisc it was originally dequeued from? I don't see what is the
problem.

My understanding of the offload intention is not really to bypass dequeue()
in general and as a matter of principle, but rather to bypass the root's
taprio_dequeue() specifically, as that could do unrelated work, and jump
right to the specific child's dequeue().

The child could have its own complex enqueue() and dequeue() and that is
perfectly fine - for example cbs_dequeue_soft() is a valid child dequeue
procedure - as long as the process isn't blocked in the sendmsg() call
by __qdisc_run() processing packets belonging to unrelated traffic
classes.