Date:	Mon, 28 Nov 2011 13:41:11 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	mathieu.desnoyers@...ymtl.ca, josh@...htriplett.org,
	niv@...ibm.com, tglx@...utronix.de, rostedt@...dmis.org,
	Valdis.Kletnieks@...edu, dhowells@...hat.com,
	eric.dumazet@...il.com, darren@...art.com, patches@...aro.org
Subject: Re: [PATCH RFC tip/core/rcu 24/28] rcu: Introduce bulk reference
 count

On Wed, 2011-11-02 at 13:30 -0700, Paul E. McKenney wrote:
> The RCU implementations, including SRCU, are designed to be used in a
> lock-like fashion, so that the read-side lock and unlock primitives must
> execute in the same context for any given read-side critical section.
> This constraint is enforced by lockdep-RCU.  However, there is a need for
> something that acts more like a reference count than a lock, in order
> to allow (for example) the reference to be acquired within the context
> of an exception, while that same reference is released in the context of
> the task that encountered the exception.  The cost of this capability is
> that the read-side operations incur the overhead of disabling interrupts.
> Some optimization is possible, and will be carried out if warranted.
> 
> Note that although the current implementation allows a given reference to
> be acquired by one task and then released by another, all known possible
> implementations that allow this have scalability problems.  Therefore,
> a given reference must be released by the same task that acquired it,
> though perhaps from an interrupt or exception handler running within
> that task's context.

I'm having trouble with the naming as well as the need for an explicit
new API.

To me this looks like a regular (S)RCU variant, nothing to do with
references per-se (aside from the fact that SRCU is a refcounted rcu
variant). Also WTF is this bulk stuff about? Its still a single ref at a
time, not 10s or 100s or whatnot.

> +static inline int bulkref_get(bulkref_t *brp)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	ret =  __srcu_read_lock(brp);
> +	local_irq_restore(flags);
> +	return ret;
> +}
> +
> +static inline void bulkref_put(bulkref_t *brp, int idx)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	__srcu_read_unlock(brp, idx);
> +	local_irq_restore(flags);
> +}

This seems to be the main gist of the patch, which to me sounds utterly
ridiculous. Why not document that srcu_read_{un,}lock() aren't IRQ safe
and if you want to use it from those contexts you have to fix it up
yourself.

RCU lockdep doesn't do the full validation so it won't actually catch it
if you mess up the irq states, but I guess if you want we could look at
adding that.

> diff --git a/kernel/srcu.c b/kernel/srcu.c
> index 73ce23f..10214c8 100644
> --- a/kernel/srcu.c
> +++ b/kernel/srcu.c
> @@ -34,13 +34,14 @@
>  #include <linux/delay.h>
>  #include <linux/srcu.h>
>  
> -static int init_srcu_struct_fields(struct srcu_struct *sp)
> +int init_srcu_struct_fields(struct srcu_struct *sp)
>  {
>  	sp->completed = 0;
>  	mutex_init(&sp->mutex);
>  	sp->per_cpu_ref = alloc_percpu(struct srcu_struct_array);
>  	return sp->per_cpu_ref ? 0 : -ENOMEM;
>  }
> +EXPORT_SYMBOL_GPL(init_srcu_struct_fields);

What do we need this export for? Usually we don't add exports unless
there's a use-case. Since Srikar requested this nonsense, I guess the
user is uprobes, but that isn't a module, so no export needed.

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

Powered by blists - more mailing lists