lkml.org 
[lkml]   [2015]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 05/10] seqlock: Better document raw_write_seqcount_latch()

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Mon, Apr 13, 2015 at 11:21:46AM -0700, Linus Torvalds wrote:
> > On Mon, Apr 13, 2015 at 10:43 AM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > A shorthand for READ_ONCE + smp_read_barrier_depends() is the shiny
> > > new lockless_dereference()
> >
> > Related side note - I think people should get used to seeing
> > "smp_load_acquire()". It has well-defined memory ordering properties
> > and should generally perform well on most architectures. It's (much)
> > stronger than lockless_dereference(), and together with
> > smp_store_release() you can make rather clear guarantees about passing
> > data locklessly from one CPU to another.
> >
> > I'd like to see us use more of the pattern of
> >
> > - one thread does:
> >
> > .. allocate/create some data
> > smp_store_release() to "expose it"
> >
> > - another thread does:
> >
> > smp_load_acquire() to read index/pointer/flag/whatever
> > .. use the data any damn way you want ..
> >
> > and we should probably aim to prefer that pattern over a lot of our
> > traditional memory barriers.
>
> I couldn't agree more!

/me too!

> RCU made a similar move from open-coding smp_read_barrier_depends()
> to using rcu_dereference() many years ago, and that change made RCU
> code -much- easier to read and understand. I believe that moving
> from smp_mb(), smp_rmb(), and smp_wmb() to smp_store_release() and
> smp_load_acquire() will provide similar maintainability benefits.
> Furthermore, when the current code uses smp_mb(),
> smp_store_release() and smp_load_acquire() generate faster code on
> most architectures.

A similar maintainability argument can be made for locking:
spin_lock(x) was a big step forward compared to lock_kernel(),
primarily not because it improves scalability (it often does), but
because the '(x)' very clearly documents the data structure that is
being accessed and makes locking and data access bugs a lot more
visible in the review phase already.

I wish rcu_read_lock() had a data argument, for similar reasons - even
if it just pointed to a pre-existing lock or an rcu head it never
touches ;-)

As an example I picked a random file out of the kernel that uses RCU:
kernel/cpuset.c::validate_change():

static int validate_change(struct cpuset *cur, struct cpuset *trial)
{
struct cgroup_subsys_state *css;
struct cpuset *c, *par;
int ret;

rcu_read_lock();

/* Each of our child cpusets must be a subset of us */
ret = -EBUSY;
cpuset_for_each_child(c, css, cur)
if (!is_cpuset_subset(c, trial))
goto out;

/* Remaining checks don't apply to root cpuset */
ret = 0;
if (cur == &top_cpuset)
goto out;

par = parent_cs(cur);

/* On legacy hiearchy, we must be a subset of our parent cpuset. */
ret = -EACCES;
if (!cgroup_on_dfl(cur->css.cgroup) && !is_cpuset_subset(trial, par))
goto out;

/*
* If either I or some sibling (!= me) is exclusive, we can't
* overlap
*/
ret = -EINVAL;
cpuset_for_each_child(c, css, par) {
if ((is_cpu_exclusive(trial) || is_cpu_exclusive(c)) &&
c != cur &&
cpumask_intersects(trial->cpus_allowed, c->cpus_allowed))
goto out;
if ((is_mem_exclusive(trial) || is_mem_exclusive(c)) &&
c != cur &&
nodes_intersects(trial->mems_allowed, c->mems_allowed))
goto out;
}

/*
* Cpusets with tasks - existing or newly being attached - can't
* be changed to have empty cpus_allowed or mems_allowed.
*/
ret = -ENOSPC;
if ((cgroup_has_tasks(cur->css.cgroup) || cur->attach_in_progress)) {
if (!cpumask_empty(cur->cpus_allowed) &&
cpumask_empty(trial->cpus_allowed))
goto out;
if (!nodes_empty(cur->mems_allowed) &&
nodes_empty(trial->mems_allowed))
goto out;
}

/*
* We can't shrink if we won't have enough room for SCHED_DEADLINE
* tasks.
*/
ret = -EBUSY;
if (is_cpu_exclusive(cur) &&
!cpuset_cpumask_can_shrink(cur->cpus_allowed,
trial->cpus_allowed))
goto out;

ret = 0;
out:
rcu_read_unlock();
return ret;
}

So just from taking a glance at that function can you tell me what is
being RCU protected here? I cannot, I can only guess that it must
either be cpuset_for_each_child() or maybe the cpumasks or other
internals.

And if I search the file for call_rcu() it shows me nothing. Only if I
know that cpusets are integrated with cgroups and I search
kernel/cgroup.c for call_rcu(), do I find:

call_rcu(&css->rcu_head, css_free_rcu_fn);

aha!

... or if I drill down 3 levels into cpuset_for_each_child() ->
css_for_each_child() -> css_next_child() do I see the RCU iteration.

It would have been a lot clearer from the onset, if I had a hint
syntactically:

rcu_read_lock(&css->rcu_head);
...
rcu_read_unlock(&css->rcu_head);

beyond the reviewer bonus I bet this would allow some extra debugging
as well (only enabled in debug kernels):

- for example to make sure we only access a field if _that field_ is
RCU locked (reducing the chance of having the right locking for
the wrong reason)

- we could possibly also build lockdep dependencies out of such
annotated RCU locking patterns.

- RCU aware list walking primitives could auto-check that this
particular list is properly RCU locked.

This could be introduced gradually by using a different API name:

rcu_lock(&css->rcu_head);
...
rcu_unlock(&css->rcu_head);

(the 'read' is implied in RCU locking anyway.)

... and if you think this approach has any merit, I volunteer the perf
and sched subsystems as guinea pigs! :-)

What do you think?

Thanks,

Ingo


\
 
 \ /
  Last update: 2015-04-14 12:41    [W:0.082 / U:1.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site