On Wed 08 Aug 2018 at 01:37, Marcelo Ricardo Leitner
<marcelo.leit...@gmail.com> wrote:
> On Mon, Aug 06, 2018 at 09:54:24AM +0300, Vlad Buslov wrote:
>> Extend rate estimator 'new' and 'replace' APIs with additional spinlock
>> parameter to be used by rtnl-unlocked actions to protect rate_est pointer
>> from concurrent modification.
>
> I'm wondering if this additional parameter is really needed. So far,
> the only case in which it is not NULL, the same lock that is used to
> protect the stats is also used in this new parameter.
>
> ...
>
>> --- a/net/sched/act_police.c
>> +++ b/net/sched/act_police.c
>> @@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct
>> nlattr *nla,
>>
>> if (est) {
>> err = gen_replace_estimator(&police->tcf_bstats, NULL,
>> - &police->tcf_rate_est,
>> + &police->tcf_rate_est, NULL,
>> &police->tcf_lock,
>> NULL, est);
>
> Which is here, and this new NULL arg is replaced by &police->tcf_lock
> in the next patch.
>
> Do you foresee a case in which a different lock will be used?
Not in my use-case, no.
> Or maybe it is because the current one is explicitly aimed towards the
> stats?
Yes, stats lock is only taken when fetching counters. You think better
approach would be to rely on the fact that, in case of police action,
same lock is already passed as stats lock? Having it as standalone
argument looked like cleaner approach to me. If you think this change is
too much code for very little benefit, I can reuse stats lock.
>
> Marcelo
Thank you for reviewing my code!
