linux-kernel - Re: [PATCHv2] lib/ratelimit: Lockless ratelimiting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfxajbTK3azfRWVBY6+f91JjGZwzHKrhQcHXDw9xeO_HA@mail.gmail.com>
Date:   Tue, 26 Jun 2018 21:41:36 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Dmitry Safonov <dima@...sta.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>, David Airlie <airlied@...ux.ie>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        "Theodore Ts'o" <tytso@....edu>,
        Thomas Gleixner <tglx@...utronix.de>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCHv2] lib/ratelimit: Lockless ratelimiting

On Tue, Jun 26, 2018 at 8:46 PM, Dmitry Safonov <dima@...sta.com> 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