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