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: Tue, 7 Aug 2018 22:37:28 -0300
Message-ID: <20180808013724.GE14666@localhost.localdomain>
References: <1533538465-23199-1-git-send-email-vladbu@mellanox.com>
 <1533538465-23199-14-git-send-email-vladbu@mellanox.com>
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-f196.google.com ([209.85.220.196]:34472 "EHLO
        mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726245AbeHHDyn (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 7 Aug 2018 23:54:43 -0400
Received: by mail-qk0-f196.google.com with SMTP id b66-v6so458589qkj.1
        for <netdev@vger.kernel.org>; Tue, 07 Aug 2018 18:37:33 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <1533538465-23199-14-git-send-email-vladbu@mellanox.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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?
Or maybe it is because the current one is explicitly aimed towards the
stats?

  Marcelo