linux-kernel - Re: [PATCH 23/26] Generic dynamic per cpu refcounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJAFBLAh81+hC=FRMMSBhP=RrwsHEW8i3PgxakqyAEPapcQaSw@mail.gmail.com>
Date:	Thu, 27 Dec 2012 14:47:49 +0100
From:	Fubo Chen <fubo.chen@...il.com>
To:	Kent Overstreet <koverstreet@...gle.com>
Cc:	linux-kernel@...r.kernel.org, linux-aio@...ck.org,
	linux-fsdevel@...r.kernel.org, zab@...hat.com, bcrl@...ck.org,
	jmoyer@...hat.com, axboe@...nel.dk, viro@...iv.linux.org.uk
Subject: Re: [PATCH 23/26] Generic dynamic per cpu refcounting

On Mon, Dec 3, 2012 at 9:58 PM, Kent Overstreet <koverstreet@...gle.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/