Re: [PATCH] percpu_counter: Fix __percpu_counter_sum()

[Eric Dumazet]

Now percpu_counter_sum() is 'fixed', what about "percpu_counter_add()" ?

void __percpu_counter_add(struct percpu_counter *fbc, s64 amount, s32 batch)
{
        s64 count;
        s32 *pcount;
        int cpu = get_cpu();

        pcount = per_cpu_ptr(fbc->counters, cpu);
        count = *pcount + amount;
        if (count >= batch || count <= -batch) {
                spin_lock(&fbc->lock);
                fbc->count += count;
                *pcount = 0;
                spin_unlock(&fbc->lock);
        } else {
                *pcount = count;
        }
        put_cpu();
}


If I read this well, this is not IRQ safe.

get_cpu() only disables preemption IMHO

For nr_files, nr_dentry, nr_inodes, it should not be a problem.

But for network counters (only in net-next-2.6) 
and lib/proportions.c, we have a problem ?

Using local_t instead of s32 for cpu 
local counter here is possible, so that fast path doesnt have 
to disable interrupts

(use a local_t instead of s32 for fbc->counters)

void __percpu_counter_add_irqsafe(struct percpu_counter *fbc, s64 amount, s32 batch)
{
        long count;
        local_t *pcount;

	/* following code only matters on 32bit arches */
	if (sizeof(amount) != sizeof(local_t)) {
		if (unlikely(amount >= batch || amount <= -batch))) {
        	        spin_lock_irqsave(&fbc->lock, flags);
                	fbc->count += amount;
                	spin_unlock_irqrestore(&fbc->lock, flags);
			return;
		}
	}
        pcount = per_cpu_ptr(fbc->counters, get_cpu());
        count = local_add_return((long)amount, pcount);
        if (unlikely(count >= batch || count <= -batch)) {
		unsigned long flags;

		local_sub(count, pcount);
                spin_lock_irqsave(&fbc->lock, flags);
                fbc->count += count;
                spin_unlock_irqrestore(&fbc->lock, flags);
        }
        put_cpu();
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/