Re: [PATCHv2] lib/ratelimit: Lockless ratelimiting

[Andy Shevchenko]

On Tue, Jun 26, 2018 at 8:46 PM, Dmitry Safonov <dima@xxxxxxxxxx> wrote:
> On Tue, 2018-06-26 at 20:04 +0300, Andy Shevchenko wrote

>> >  #define RATELIMIT_STATE_INIT(name, interval_init, burst_init)
>> > {                \
>> > -               .lock           =
>> > __RAW_SPIN_LOCK_UNLOCKED(name.lock),  \
>>
>> name is now redundant, isn't it?
>
> It is. Worth to split on the second patch or keep callers changes in
> this patch?

For me sounds like a part of this change, though weakly tighten to the
main purpose.
Otherwise in the middle of the series you introduce some bogus stuff
(not sure if compiler or some static analyzers would warn about it).

>> > @@ -42,9 +41,10 @@ static inline void ratelimit_state_init(struct
>> > ratelimit_state *rs,
>> >  {
>> >         memset(rs, 0, sizeof(*rs));
>> >
>> > -       raw_spin_lock_init(&rs->lock);
>> >         rs->interval    = interval;
>> >         rs->burst       = burst;
>> > +       atomic_set(&rs->printed, 0);
>> > +       atomic_set(&rs->missed, 0);
>>
>> Can it be
>>
>> *rs = RATELIMIT_STATE_INIT(interval, burst);
>>
>> ?
>>
>> (Yes, the '(struct ratelimit_state)' has to be added to macro to
>> allow this)
>
> Sure.

This part, by the way, potentially can be split into preparatory
patch. Please, double check if it possible to do this way.

>> > -       if (rs->missed) {
>> > +       if ((missed = atomic_xchg(&rs->missed, 0)))
>>
>> Perhaps
>>
>> missed = ...
>> if (missed)
>>
>> ?
>
> Ok, will change - checkpatch has warned me, but I thought it's just a
> preference than a rule.

In general, yes and no. In this case it would increase readability and
assignment inside conditionals is not the best practice.

>> Instead of casting, perhaps
>>
>> int missed = ...
>>
>> I think you already has a guard against going it below zero. Or I
>> missed something?
>
> No, I do:
> atomic_add_unless(&rs->missed, 1, -1);
>
> So, it's guard against overflow, but not against negative.
> That's why I do print it as unsigned.

Hmm...
If you increment the variable, it would become 2^n, then 2^n + 1, ...
which in unsigned form quite far from -1.

So, this check is against revolving the value.
Otherwise you are using atomic_t as unsigned type indeed.

But in case of use you assign signed to unsigned which would not
overflow, so, the casting is superfluous.

(and I would rather go with unsigned int type instead of unsigned if
it fits style of the module)

-- 
With Best Regards,
Andy Shevchenko