[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-aio
Subject: Re: [PATCH 23/26] Generic dynamic per cpu refcounting
From: Fubo Chen <fubo.chen () gmail ! com>
Date: 2012-12-27 13:47:49
Message-ID: CAJAFBLAh81+hC=FRMMSBhP=RrwsHEW8i3PgxakqyAEPapcQaSw () mail ! gmail ! com
[Download RAW message or body]
On Mon, Dec 3, 2012 at 9:58 PM, Kent Overstreet <koverstreet@google.com> wrote:
> +#define pr_fmt(fmt) "%s: " fmt "\n", __func__
Dynamic debug already allows to insert the function name. Please
consider leaving this line out entirely and move the "\n" to the end
of individual printed lines.
> +#define PCPU_REF_PTR 0
> +#define PCPU_REF_NONE 1
> +#define PCPU_REF_DYING 2
> +#define PCPU_REF_DEAD 3
Why has "define" been used here instead of an enum ? Using "define"
prevents the compiler to verify whether all possible values have been
handled in a switch statement.
> +#define REF_STATUS(count) ((unsigned long) count & PCPU_STATUS_MASK)
Please leave out the cast in the above expression. It prevents that
the compiler performs type checking on "count". Also, why is
REF_STATUS() a define and not an inline function ?
> +struct percpu_ref {
> + atomic64_t count;
> + unsigned __percpu *pcpu_count;
> +};
pcpu_count can either be a jiffies value (unsigned long) or a pointer
to unsigned __percpu. So please consider using a union here. Has this
code been checked with sparse ? I'm afraid that the above structure
definition will confuse sparse a lot.
> + if (now - last <= HZ << PCPU_STATUS_BITS) {
Please consider changing "HZ" into a symbolic name that makes it clear
that this value is the threshold in jiffies above which the switch is
made from a single atomic_t refcount to the per-cpu refcounting
scheme.
> +void __percpu_ref_get(struct percpu_ref *ref, bool alloc)
> +{
> + unsigned __percpu *pcpu_count;
> + uint64_t v;
> +
> + pcpu_count = rcu_dereference(ref->pcpu_count);
> +
> + if (REF_STATUS(pcpu_count) == PCPU_REF_PTR) {
> + __this_cpu_inc(*pcpu_count);
> + } else {
> + v = atomic64_add_return(1 + (1ULL << PCPU_COUNT_BITS),
> + &ref->count);
> +
> + if (!(v >> PCPU_COUNT_BITS) &&
> + REF_STATUS(pcpu_count) == PCPU_REF_NONE && alloc)
> + percpu_ref_alloc(ref, pcpu_count);
> + }
> +}
What will happen if another thread invokes percpu_ref_kill() after the
above rcu_dereference() call finished but before REF_STATUS() got
invoked ? Can that percpu_ref_kill() finish before
__this_cpu_inc(*pcpu_count) gets invoked ?
Documentation/RCU/checklist.txt says: "All RCU list-traversal
primitives, which include rcu_dereference(),
list_for_each_entry_rcu(), and list_for_each_safe_rcu(), must be
either within an RCU read-side critical section or must be protected
by appropriate update-side locks."
> +int percpu_ref_kill(struct percpu_ref *ref)
The meaning of the return value of this function has not been documented.
Thanks,
Fubo.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic