netdev - Re: [RFC 2/3] tc: deprecate TC_ACT

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: <CAHA+R7NpbQaWdXzdu24FZdRFn1VJvf6q14JyfCpD2dXkx-mz6w@mail.gmail.com>
Date:	Thu, 23 Apr 2015 20:37:58 -0700
From:	Cong Wang <cwang@...pensource.com>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	Jamal Hadi Salim <jhs@...atatu.com>,
	"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 Thu, Apr 23, 2015 at 5:59 PM, Alexei Starovoitov <ast@...mgrid.com> wrote:
> 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.

Which performance problem did you see? Did you hit the spinlock
bottleneck? That spinlock should be eliminated before you
try to do more, and it can be replaced by RCU read lock after
John's work (I am still waiting for his patch btw).


> 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.

Nope, it breaks Qdisc abstraction, filters never attach to netdevice
directly. You should stop, since you really don't understand the code.
--
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