Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 09/11] drivers/char/ipmi: convert stats to use
 counter_atomic32
To:     minyard@acm.org
Cc:     arnd@arndb.de, gregkh@linuxfoundation.org, keescook@chromium.org,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <cover.1601073127.git.skhan@linuxfoundation.org>
 <b742c2637a8b6083c471a6dd829db70071909f83.1601073127.git.skhan@linuxfoundation.org>
 <20200926001510.GH3674@minyard.net>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <dc07811e-d8e9-f9dd-bc6a-1877d7f06344@linuxfoundation.org>
Date:   Fri, 25 Sep 2020 20:05:39 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200926001510.GH3674@minyard.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/25/20 6:15 PM, Corey Minyard wrote:
> On Fri, Sep 25, 2020 at 05:47:23PM -0600, Shuah Khan wrote:
>> counter_atomic* is introduced to be used when a variable is used as
>> a simple counter and doesn't guard object lifetimes. This clearly
>> differentiates atomic_t usages that guard object lifetimes.
>>
>> counter_atomic* variables will wrap around to 0 when it overflows and
>> should not be used to guard resource lifetimes, device usage and
>> open counts that control state changes, and pm states.
>>
>> atomic_t variables used for stats are atomic counters. Overflow will
>> wrap around and reset the stats and no change with the conversion.
>>
>> Convert them to use counter_atomic32.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> Reviewed-by: Corey Minyard <cminyard@mvista.com>
> 

Thanks Corey.

> I assume for this conversion that the plan is to eliminate atomic_t
> completely and convert all atomic counters used for object lifetime to
> struct kref?  The new naming is certainly more clear and I'm happy with
> this change.
> 

No plans to replace or get rid of atomic_t/refcount_t ops. The reason is
to clearly differentiate atomic_t uses that don't guard object lifetimes
or state management, hence prone to overflow and underflow errors.

By clearly differentiating the ones guard the lifetimes and that don't
using this new counter interface, the existing tools that scan for
overflow/underflow conditions can filter out Counters API and look for
the variables that truly guard the lifetimes. Currently it is becoming
very hard to zero in on the errors with the noise.

Second reason is, atomic_t is overused. Non-atomic counters in this
API can be used for stats/counters that don't need atomocity.

Hope this helps.

thanks,
-- Shuah