Re: [RFC][PATCH 1/3] per cpu counter fixes for unsigned long typecounter overflow

[Andrew Morton]

Mingming Cao <cmm@xxxxxxxxxx> wrote:
>
> [PATCH 1/3] - Currently the"long" type counter maintained in percpu
> counter could have issue when handling a counter that is a unsigned long
> type. Most cases this could be easily fixed by casting the returned
> value to "unsigned long". But for the "overflow" issue, i.e. because of
> the percpu global counter is a approsimate value, there is a
> possibility that at some point the global counter is close to the max
> limit (oxffff_feee) but after updating from a local counter a positive
> value, the global counter becomes a small value (i.e.0x 00000012). 
> 
> This patch tries to avoid this overflow happen. When updating from a
> local counter to the global counter, add a check to see if the updated
> value is less than before if we are doing an positive add, or if the
> updated value is greater than before if we are doing an negative add.
> Either way we should postpone the updating from this local counter to
> the global counter.
> 
>  
> -static void __percpu_counter_mod(struct percpu_counter *fbc, long amount)
> +static void __percpu_counter_mod(struct percpu_counter *fbc, long amount,
> +				int llcheck)

Confused.  What does "ll" stand for throughout this patch?

Whatever it is, I suspect we need to choose something better ;)

>  {
>  	long count;
>  	long *pcount;
>  	int cpu = smp_processor_id();
> +	unsigned long before, after;
> +	int update = 1;
>  
>  	pcount = per_cpu_ptr(fbc->counters, cpu);
>  	count = *pcount + amount;
>  	if (count >= FBC_BATCH || count <= -FBC_BATCH) {
>  		spin_lock(&fbc->lock);
> -		fbc->count += count;
> -		*pcount = 0;
> +		before = fbc->count;
> +		after = before + count;
> +		if (llcheck && ((count > 0 && after < before) ||
> +				( count < 0 && after > before)))
> +			update = 0;
> +
> +		if (update) {
> +			fbc->count = after;
> +			*pcount = 0;
> +		}

The above bit of magic deserves an explanatory comment.

>  		spin_unlock(&fbc->lock);
>  	} else {
>  		*pcount = count;
>  	}
>  }
>  
> +void percpu_counter_mod_ll(struct percpu_counter *fbc, long amount)
> +{
> +	preempt_disable();
> +	__percpu_counter_mod(fbc, amount, 1);
> +	preempt_enable();
> +}
> +EXPORT_SYMBOL(percpu_counter_mod_ll);

An introductory comment which describes the difference between this and
percpu_counter_mod() would be helpful, please.


-
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/