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

[John Fastabend <john.fastabend@gmail.com>]

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