lkml.org 
[lkml]   [2015]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH-v2 2/4] target: Drop lun_sep_lock for se_lun->lun_se_dev RCU usage
From
Date
On Thu, 2015-05-28 at 08:57 -0700, Paul E. McKenney wrote:
> On Wed, May 27, 2015 at 11:02:10PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2015-05-27 at 14:04 -0700, Paul E. McKenney wrote:
> > > On Tue, May 26, 2015 at 10:29:45PM -0700, Nicholas A. Bellinger wrote:

<SNIP>

> > > > In this particular case, the se_device behind se_lun->lun_se_dev
> > > > __rcu protected pointer can't be released without first releasing the
> > > > pre-existing se_lun->lun_group reference to se_device->dev_group.
> > > >
> > > > And since se_lun->lun_group is the source of a configfs symlink to
> > > > se_lun_acl->se_lun_group here, the se_lun associated RCU pointer and
> > > > underlying se_device can't be released out from under the above
> > > > target_fabric_mappedlun_link() code accessing a __rcu protected pointer.
> > > >
> > > > Paul, is lockless_dereference the correct notation for this type of
> > > > use-case..?
> > >
> > > My guess is "no", but I don't claim to understand your use case.
> > >
> > > The splat is against some other code than the patch, judging by the
> > > patch line numbers.
> > >
> > > The rule is that if a pointer points to something that is freed (or
> > > reused) after a grace period, you mark that pointer with __rcu.
> > > Any access to that pointer must then be accessed in an RCU read-side
> > > critical section, using one of the RCU list iterators or one of the
> > > rcu_dereference() macros. No lockless_dereference() in this case.
> > >
> > > You use lockless_dereference() when something other than RCU controls
> > > when the pointer target is freed.
> >
> > For this case, there is a pointer with __rcu notation being
> > dereferenced, but given the way configfs parent/child config_group
> > reference counting works, it's impossible for this __rcu pointer to be
> > modified, and impossible for RCU updater path (-> kfree_rcu) of the
> > structure being dereferenced to run, while this particular code is
> > executed.
> >
> > So I was thinking this should be using something like
> > rcu_dereference_protected(), but from the comment it sounds like this is
> > intended only for RCU updater path code.
>
> If something is preventing the pointer from changing, then it is OK
> to use rcu_dereference_protected(). If the pointer might change, then
> you are right, you absolutely cannot use rcu_dereference_protected(),
> as it does not protect against concurrent updates.
>
> If reasonably possible, you should pass a reference-held expression to
> rcu_dereference_protected().
>
> > Is there some other notation to use for this type of case where the RCU
> > updater path can't run due to external reference counting, or should
> > this not be using __rcu notation at all..?
>
> You should be OK with rcu_dereference_protected(). However, for
> rcu_dereference_protected() to work properly, it must be the case
> that the pointer it is reading doesn't change.
>
> So you do have to be a bit careful. For example, if structure A has
> a reference held so that it cannot be removed at the moment, but if it
> points to some structure B that -can- be removed, then you cannot use
> rcu_dereference_protected() to access the pointer from A to B because
> that pointer could change.
>
> For another example, assume that structures C and D both have references
> held (and thus cannot be removed), and that structure C points to
> structure D. But if a structure E could be inserted between C and D,
> we -cannot- use rcu_dereference_protected() because the pointer from
> C to D could change at any time, despite both C and D being nailed down.
>
> In other words, the distinction is whether or not a given pointer can
> change, not whether or not the enclosing structure is guaranteed to live.
>
> Make sense?
>

Most certainly. Thanks for the explanation here, it's very helpful.

Ok, so converting the bogus lockless_dereference() usage to:

- rcu_dereference_check() when called from a read-critical path to
include the necessary smp_read_barrier_depends() + ACCESS_ONCE(),
when RCU updater side can potentially execute.
- rcu_dereference_protected() when called from an updater path with
a lock held.
- rcu_dereference_protected() when called from a reader path that can
only run while the updater side cannot execute due to external
reference counting.
- rcu_dereference_raw() for other special cases where a reference
can't be verified, with an appropriate comment.

Thank you,

--nab



\
 
 \ /
  Last update: 2015-05-31 07:41    [W:0.071 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site