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: <5539956D.5070506@plumgrid.com>
Date:	Thu, 23 Apr 2015 17:59:25 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <cwang@...pensource.com>
CC:	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	John Fastabend <john.r.fastabend@...el.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

On 4/23/15 3:51 PM, Jamal Hadi Salim wrote:
>
> So you are planning to add queues? If you are that is a different
> discussion (and the use case needs some clarity).

nope. I wasn't planning to do that.

 > For packets being forwarded we already had egress qdiscs which had
 > queues so it didnt seem to make sense to enqueue on ingress for such
 > use cases.
 > There was a use case later where multiple ingress ports had to be
 > shared
 > and ifb was born - which is pseudo temporary enqueueing on ingress.

agree. imo ifb approach is more flexible, since it has full hierarchy
of qdiscs. As you're saying above and from the old ifb logs I thought
that ifb is _temporary_ and long term plan is to use ingress_queue,
but looks like this is not the case. Also not too long ago we decided
that we don't want another ingress qdisc. Therefore I think it makes
sense to simplify the code and boost performance.
I'm not proposing to change tooling, of course.
 From iproute2 point of view we'll still have ingress qdisc.
Right now we're pointlessly allocating memory for it and for
ingress_queue, whereas we only need to call cls/act.
I'm proposing to kill them and used tcf_proto in net_device instead.
Right now to reach cls in critical path on ingress we do:
rxq = skb->dev->ingress_queue
sch = rxq->qdisc
sch->enqueue
sch->filter_list
with a bunch of 'if' conditions and useless memory accesses in-between.
If we add 'ingress_filter_list' directly to net_device,
it will be just:
skb->dev->ingress_filter_list
which is huge performance boost. Code size will shrink as well.
iproute2 and all existing tools will work as-is. Looks as win-win to me.

>>> The fact that qdiscs dealt with these codes directly
>>> allows for specialized handling. Moving them to a generic function
>>> seems to defeat that purpose. So I am siding with Cong on this.
>>
>> that's not what patch 1 is doing. It is still doing specialized
>> handling... but in light of what you said above, it looks like much
>> bigger cleanup is needed. We'll continue arguing when I refactor
>> this set and resubmit when net-next reopens.
>
> Sorry - i was refereing to the patch where you agregated things after
> the qdisc invokes a classifier.

hmm. There I also didn't convert all qdiscs into single helper.
Only those that have exactly the same logic after tc_classify.
All other qdiscs have custom handling.
No worries, it's hard to review things while traveling. Been there :)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ