netdev - Re: [RCU PATCH 00/14] Remove qdisc lock around ingress Qdisc

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: <53208F11.8010304@gmail.com>
Date:	Wed, 12 Mar 2014 09:45:05 -0700
From:	John Fastabend <john.fastabend@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>
CC:	xiyou.wangcong@...il.com, netdev@...r.kernel.org,
	davem@...emloft.net
Subject: Re: [RCU PATCH 00/14] Remove qdisc lock around ingress Qdisc

On 03/11/2014 11:58 PM, Jamal Hadi Salim wrote:
> On 03/10/14 13:03, John Fastabend wrote:
>> This series drops the qdisc lock that is currently protecting the
>> ingress qdisc. This can be done after the tcf filters are made
>> lockless and the statistic accounting is safe to run without locks.
>>
>> To do this the classifiers are converted to use RCU. This requires
>> updating each classifier individually to handle the new copy/update
>> requirement and also to update the core list traversals. This is
>> done in patches 2-11. This also makes the assumption that updates
>> to the tables are infrequent in comparison to the packet per second
>> being classified. On a 10Gbps running near line rate we can easily
>> produce 12+ million packets per second so IMO this is a reasonable
>> assumption. And the updates are serialized by RTNL.
>>
>
>
> I am in travel mode and dont have much cycles to do full review
> and the patch set seems to be based on what we discussed earlier.
> I worry on whether the assumption that table updates being
> infrequent is going to cripple some of the use cases that have
> made the interface powerful.  Cant find a presentation i did a
> while back nor my data (i will look when i get back)- but one of the
> things we prided on was ability to do extremely fast updates (both
> latency and throughput) even in presence of datapath activity.
> My comparison at the time was against netfilter (we were about
> a magnitude faster).
> So a good metric is to pick some classifier and action - then do
> table updates with no traffic as a base metric and with your changes
> after. Likewise with traffic.

Sure I can provide this data as part of the patch description. My
expectation is now that the qdisc lock is not needed there should
be less impact to throughput/latency due to filter updates. But I'll
produce the data.

>
> BTW: does this mean there may be lack of sync between what the
> control update is doing and what is being used in the datapath?

Yes upto one RCU grace period. Although I don't think this is an
issue because we get consistency eventually and even with the qdisc
lock there is no way to know if a set of skbs hit the filter list
before or after the update.

I'll get a v2 out tomorrow morning after making Eric's changes and
fixing the last compiler warning.

> net/core/gen_estimator.c: In function ‘gen_get_bstats’:
> net/core/gen_estimator.c:163:43: warning: pointer type mismatch in conditional expression [enabled by default]
> net/core/gen_estimator.c: In function ‘gen_find_node’:
> net/core/gen_estimator.c:191:28: warning: pointer type mismatch in conditional expression [enabled by default]

compiler doesn't like the usage of voids here.

Thanks,
John


>
> cheers,
> jamal
>
>
>


-- 
John Fastabend         Intel Corporation
--
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