netdev - Re: [patch net-next 06/34] net: core: use dev->ingress

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: <20171015064535.GA1970@nanopsycho.orion>
Date:   Sun, 15 Oct 2017 08:45:35 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, mlxsw@...lanox.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        michael.chan@...adcom.com, ganeshgr@...lsio.com,
        jeffrey.t.kirsher@...el.com, saeedm@...lanox.com,
        matanb@...lanox.com, leonro@...lanox.com, idosch@...lanox.com,
        jakub.kicinski@...ronome.com, ast@...nel.org,
        simon.horman@...ronome.com, pieter.jansenvanvuuren@...ronome.com,
        john.hurley@...ronome.com, edumazet@...gle.com, dsahern@...il.com,
        alexander.h.duyck@...el.com, john.fastabend@...il.com,
        willemb@...gle.com
Subject: Re: [patch net-next 06/34] net: core: use dev->ingress_queue instead
 of tp->q

Sun, Oct 15, 2017 at 01:18:54AM CEST, daniel@...earbox.net wrote:
>On 10/13/2017 08:30 AM, Jiri Pirko wrote:
>> Thu, Oct 12, 2017 at 11:45:43PM CEST, daniel@...earbox.net wrote:
>> > On 10/12/2017 07:17 PM, Jiri Pirko wrote:
>> > > From: Jiri Pirko <jiri@...lanox.com>
>> > > 
>> > > In sch_handle_egress and sch_handle_ingress, don't use tp->q and use
>> > > dev->ingress_queue which stores the same pointer instead.
>> > > 
>> > > Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>> > > ---
>> > >    net/core/dev.c | 21 +++++++++++++++------
>> > >    1 file changed, 15 insertions(+), 6 deletions(-)
>> > > 
>> > > diff --git a/net/core/dev.c b/net/core/dev.c
>> > > index fcddccb..cb9e5e5 100644
>> > > --- a/net/core/dev.c
>> > > +++ b/net/core/dev.c
>> > > @@ -3273,14 +3273,18 @@ EXPORT_SYMBOL(dev_loopback_xmit);
>> > >    static struct sk_buff *
>> > >    sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
>> > >    {
>> > > +	struct netdev_queue *netdev_queue =
>> > > +				rcu_dereference_bh(dev->ingress_queue);
>> > >    	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
>> > >    	struct tcf_result cl_res;
>> > > +	struct Qdisc *q;
>> > > 
>> > > -	if (!cl)
>> > > +	if (!cl || !netdev_queue)
>> > >    		return skb;
>> > > +	q = netdev_queue->qdisc;
>> > 
>> > NAK, no additional overhead in the software fast-path of
>> > sch_handle_{ingress,egress}() like this. There are users out there
>> > that use tc in software only, so performance is critical here.
>> 
>> Okay, how else do you suggest I can avoid the need to use tp->q?
>> I was thinking about storing q directly to net_device, which would safe
>> one dereference, resulting in the same amount as current cl->q.
>
>Sorry for late reply, mostly off for few days. netdev struct has different
>cachelines which are hot on rx and tx (see also the location of the two
>lists, ingress_cl_list and egress_cl_list), if you add only one qdisc
>pointer there, then you'd at least penalize one of the two w/ potential
>cache miss. Can we leave it in cl?

Well that is the whole point of this excercise, to remove it from cl.
The thing is, for the shared blocks, cls are shared between multiple
qdisc instances, so cl->q makes no longer any sense.