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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
