Re: [RFC][PATCH 0/5] Introduce checks for preemptable code forthis_cpu_read/write()

[Steven Rostedt]

On Tue, 2011-09-20 at 14:12 -0400, Mathieu Desnoyers wrote:
> Not quite. What I was proposing more precisely:
> 
> - this_cpu_*() for the case where the caller needs to disable
>   preemption. This is the default case. This is exactly what you
>   proposed, with WARN_ON debug checks. This could even be "percpu_*()"
>   now that I think of it. There is no real point in the "this_cpu"
>   prefix.
> 
> - preempt_protected_percpu_*() and irq_protected_percpu_*() for
>   statistics/slub use. Those primitives disable preemption or irq
>   internally on non-x86 architectures. The caller of these primitives
>   is not required to disable preemption nor irqs.

This is totally confusing. It suggests to me that the percpu requires
preemption protected. You are coupling the implementation of the
function too much with the name. The name should describe its use. What
does "preempt_protected" mean?  To me, it sounds like I should use this
in preempt protected mode. Still way too confusing.

any_cpu_*() is still much more understanding. It means that we are
manipulating a CPU variable, and we do not care which one.

Looking at the real use cases of this_cpu(), that seems to be exactly
the use case for it. That is, we modify the cpu variable, maybe we get
migrated, but in the end, we just read all the cpu variables and report
the net sum. Thus the design POV is that we do not care what CPU
variable we read/write. From an implementation point of view, it just
happens to be an optimization that we try to read/write to the current
cpu pointer. But in reality it doesn't matter what CPU variable we
touch.

Do not confuse implementation and optimizations with design. The big
picture design is that we do not care what CPU variable is touched. The
name should reflect that.

-- Steve


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