From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Subject: Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator
 lock parameter
Date: Wed, 8 Aug 2018 11:18:17 -0300
Message-ID: <20180808141815.GF14666@localhost.localdomain>
References: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
 <1533538465-23199-14-git-send-email-vladbu@mellanox.com>
 <20180808013724.GE14666@localhost.localdomain>
 <vbfo9ed2g7i.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com,
        xiyou.wangcong@gmail.com, jiri@resnulli.us, pablo@netfilter.org,
        kadlec@blackhole.kfki.hu, fw@strlen.de, ast@kernel.org,
        daniel@iogearbox.net, edumazet@google.com, keescook@chromium.org
To: Vlad Buslov <vladbu@mellanox.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-qk0-f193.google.com ([209.85.220.193]:37620 "EHLO
        mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727109AbeHHQiN (ORCPT
        <rfc822;netdev@vger.kernel.org>); Wed, 8 Aug 2018 12:38:13 -0400
Received: by mail-qk0-f193.google.com with SMTP id t79-v6so1611987qke.4
        for <netdev@vger.kernel.org>; Wed, 08 Aug 2018 07:18:21 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <vbfo9ed2g7i.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Wed, Aug 08, 2018 at 03:30:41PM +0300, Vlad Buslov wrote:
> On Wed 08 Aug 2018 at 01:37, Marcelo Ricardo Leitner <marcelo.leitner@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

And the fact that we have no foreseeable user of two different locks.

> 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.

That's my current thinking, yes. Especially considering the amount of
parameters this function already has, I would refrain from adding yet
another unless really needed.

Maybe s/stats_lock/lock/ in function parameter (struct member doesn't
need to be changed) and doctext:
 * @lock: lock for statistics and control path.

wdyt?

> 
> >
> >   Marcelo
> 
> Thank you for reviewing my code!