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]

Date:   Mon, 16 Oct 2017 22:20:04 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jiri Pirko <jiri@...nulli.us>
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

On 10/15/2017 08:45 AM, Jiri Pirko wrote:
> 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.

I don't really mind if you want to remove cl->q, but lets do it without
adding any cost to the fast path. The primary motivation for this set
is hw offload, but it shouldn't be at the expense to make sw fast path
slower. cl->q is only used here for updating stats, one possible option
could be to take them out for the clsact case into dev at the expense
for two pointers (e.g. getting rid of ingress_queue would reduce it to
only one pointer), such that you have percpu {ingress,egress}_cl_stats
that control path can fetch instead when dumping qdiscs. Could be one,
but there are probably other options as well to explore.

Thanks,
Daniel