On Sat, Nov 18, 2006 at 11:15:27AM -0500, Alan Stern wrote:
> There are a few things I don't like about this patch.
>
> On Fri, 17 Nov 2006, Paul E. McKenney wrote:
>
> > diff -urpNa -X dontdiff linux-2.6.19-rc5/kernel/srcu.c linux-2.6.19-rc5-dsrcu/kernel/srcu.c
> > --- linux-2.6.19-rc5/kernel/srcu.c 2006-11-17 13:54:17.000000000 -0800
> > +++ linux-2.6.19-rc5-dsrcu/kernel/srcu.c 2006-11-17 14:15:06.000000000 -0800
> > @@ -34,6 +34,18 @@
> > #include <linux/smp.h>
> > #include <linux/srcu.h>
> >
> > +/*
> > + * Initialize the per-CPU array, returning the pointer.
> > + */
> > +static inline struct srcu_struct_array *alloc_srcu_struct_percpu(void)
> > +{
> > + struct srcu_struct_array *sap;
> > +
> > + sap = alloc_percpu(struct srcu_struct_array);
> > + smp_wmb();
> > + return (sap);
>
> Style: Don't use () here.
Touche!!!
> > +}
> > +
> > /**
> > * init_srcu_struct - initialize a sleep-RCU structure
> > * @sp: structure to initialize.
>
> > @@ -94,7 +112,8 @@ void cleanup_srcu_struct(struct srcu_str
> > WARN_ON(sum); /* Leakage unless caller handles error. */
> > if (sum != 0)
> > return;
> > - free_percpu(sp->per_cpu_ref);
> > + if (sp->per_cpu_ref != NULL)
> > + free_percpu(sp->per_cpu_ref);
>
> Now that Andrew has accepted the "allow free_percpu(NULL)" change, you can
> remove the test here.
OK. I thought that there was some sort of error-checking involved,
but if not, will fix.
> > sp->per_cpu_ref = NULL;
> > }
> >
> > @@ -105,18 +124,39 @@ void cleanup_srcu_struct(struct srcu_str
> > * Counts the new reader in the appropriate per-CPU element of the
> > * srcu_struct. Must be called from process context.
> > * Returns an index that must be passed to the matching srcu_read_unlock().
> > + * The index is -1 if the srcu_struct is not and cannot be initialized.
> > */
> > int srcu_read_lock(struct srcu_struct *sp)
> > {
> > int idx;
> > + struct srcu_struct_array *sap;
> >
> > preempt_disable();
> > idx = sp->completed & 0x1;
> > - barrier(); /* ensure compiler looks -once- at sp->completed. */
> > - per_cpu_ptr(sp->per_cpu_ref, smp_processor_id())->c[idx]++;
> > - srcu_barrier(); /* ensure compiler won't misorder critical section. */
> > + sap = rcu_dereference(sp->per_cpu_ref);
> > + if (likely(sap != NULL)) {
> > + barrier(); /* ensure compiler looks -once- at sp->completed. */
>
> Put this barrier() back where the old one was (outside the "if").
Why? Outside this "if", I don't use "sap".
> > + per_cpu_ptr(rcu_dereference(sap),
>
> You don't need the rcu_dereference here, you already have it above.
Good point!!!
> > + smp_processor_id())->c[idx]++;
> > + smp_mb();
> > + preempt_enable();
> > + return idx;
> > + }
> > + if (mutex_trylock(&sp->mutex)) {
> > + preempt_enable();
>
> Move the preempt_enable() before the "if", then get rid of the
> preempt_enable() after the "if" block.
No can do. The preempt_enable() must follow the increment and
the memory barrier, otherwise the synchronize_sched() inside
synchronize_srcu() can't do its job.
> > + if (sp->per_cpu_ref == NULL)
> > + sp->per_cpu_ref = alloc_srcu_struct_percpu();
>
> It would be cleaner to put the mutex_unlock() and closing '}' right here.
I can move the mutex_unlock() to this point, but I cannot otherwise
merge the two following pieces of code -- at least not without doing
an otherwise-gratuitous preempt_disable(). Which I suppose I could
do, but seems like it would be more confusing than would the
separate code. I will play with this a bit and see if I can eliminate
the duplication.
> > + if (sp->per_cpu_ref == NULL) {
> > + atomic_inc(&sp->hardluckref);
> > + mutex_unlock(&sp->mutex);
> > + return -1;
> > + }
> > + mutex_unlock(&sp->mutex);
> > + return srcu_read_lock(sp);
> > + }
OK, I suppose I could put the preempt_enable() in an "else" clause,
then maybe be able to merge things. Would that help?
> > preempt_enable();
> > - return idx;
> > + atomic_inc(&sp->hardluckref);
> > + return -1;
> > }
> >
> > /**
> > @@ -131,10 +171,17 @@ int srcu_read_lock(struct srcu_struct *s
> > */
> > void srcu_read_unlock(struct srcu_struct *sp, int idx)
> > {
> > - preempt_disable();
> > - srcu_barrier(); /* ensure compiler won't misorder critical section. */
> > - per_cpu_ptr(sp->per_cpu_ref, smp_processor_id())->c[idx]--;
> > - preempt_enable();
> > + if (likely(idx != -1)) {
> > + preempt_disable();
> > + smp_mb();
> > + per_cpu_ptr(rcu_dereference(sp->per_cpu_ref),
> > + smp_processor_id())->c[idx]--;
> > + preempt_enable();
> > + return;
> > + }
> > + mutex_lock(&sp->mutex);
> > + atomic_dec(&sp->hardluckref);
> > + mutex_unlock(&sp->mutex);
>
> You don't need the mutex to protect an atomic_dec.
Good point!!!
> > }
> >
> > /**
> > @@ -158,6 +205,11 @@ void synchronize_srcu(struct srcu_struct
> > idx = sp->completed;
> > mutex_lock(&sp->mutex);
> >
> > + /* Initialize if not already initialized. */
> > +
> > + if (sp->per_cpu_ref == NULL)
> > + sp->per_cpu_ref = alloc_srcu_struct_percpu();
>
> What happens if a prior reader failed to allocate the memory but this call
> succeeds? You need to check hardluckref before doing this. The same is
> true in srcu_read_lock().
All accounted for by the fact that hardluckref is unconditionally
added in by srcu_readers_active(). Right?
> > +
> > /*
> > * Check to see if someone else did the work for us while we were
> > * waiting to acquire the lock. We need -two- advances of
> > @@ -173,65 +225,25 @@ void synchronize_srcu(struct srcu_struct
> > return;
> > }
> >
> > - synchronize_sched(); /* Force memory barrier on all CPUs. */
> > -
> > - /*
> > - * The preceding synchronize_sched() ensures that any CPU that
> > - * sees the new value of sp->completed will also see any preceding
> > - * changes to data structures made by this CPU. This prevents
> > - * some other CPU from reordering the accesses in its SRCU
> > - * read-side critical section to precede the corresponding
> > - * srcu_read_lock() -- ensuring that such references will in
> > - * fact be protected.
> > - *
> > - * So it is now safe to do the flip.
> > - */
> > -
> > + smp_mb(); /* ensure srcu_read_lock() sees prior change first! */
> > idx = sp->completed & 0x1;
> > sp->completed++;
> >
> > - synchronize_sched(); /* Force memory barrier on all CPUs. */
> > + synchronize_sched();
> >
> > /*
> > * At this point, because of the preceding synchronize_sched(),
> > * all srcu_read_lock() calls using the old counters have completed.
> > * Their corresponding critical sections might well be still
> > * executing, but the srcu_read_lock() primitives themselves
> > - * will have finished executing.
> > + * will have finished executing. The "old" rank of counters
> > + * can therefore only decrease, never increase in value.
> > */
> >
> > while (srcu_readers_active_idx(sp, idx))
> > schedule_timeout_interruptible(1);
> >
> > - synchronize_sched(); /* Force memory barrier on all CPUs. */
> > -
> > - /*
> > - * The preceding synchronize_sched() forces all srcu_read_unlock()
> > - * primitives that were executing concurrently with the preceding
> > - * for_each_possible_cpu() loop to have completed by this point.
> > - * More importantly, it also forces the corresponding SRCU read-side
> > - * critical sections to have also completed, and the corresponding
> > - * references to SRCU-protected data items to be dropped.
> > - *
> > - * Note:
> > - *
> > - * Despite what you might think at first glance, the
> > - * preceding synchronize_sched() -must- be within the
> > - * critical section ended by the following mutex_unlock().
> > - * Otherwise, a task taking the early exit can race
> > - * with a srcu_read_unlock(), which might have executed
> > - * just before the preceding srcu_readers_active() check,
> > - * and whose CPU might have reordered the srcu_read_unlock()
> > - * with the preceding critical section. In this case, there
> > - * is nothing preventing the synchronize_sched() task that is
> > - * taking the early exit from freeing a data structure that
> > - * is still being referenced (out of order) by the task
> > - * doing the srcu_read_unlock().
> > - *
> > - * Alternatively, the comparison with "2" on the early exit
> > - * could be changed to "3", but this increases synchronize_srcu()
> > - * latency for bulk loads. So the current code is preferred.
> > - */
> > + smp_mb(); /* must see critical section prior to srcu_read_unlock() */
> >
> > mutex_unlock(&sp->mutex);
> > }
Will spin a new patch...
Thanx, Paul
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
