On Mon, Mar 24, 2014 at 12:02:32AM +0530, Monam Agarwal wrote:
> This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL)
>
> The rcu_assign_pointer() ensures that the initialization of a structure
> is carried out before storing a pointer to that structure.
> And in the case of the NULL pointer, there is no structure to initialize.
> So, rcu_assign_pointer(p, NULL) can be safely converted to
> RCU_INIT_POINTER(p, NULL)
>
> Signed-off-by: Monam Agarwal <monamagarwal...@gmail.com>
Sounds right ... but doing this all over the place seems
fragile, and error prone. Can't we make this kind of
optimization automatic? See below:
> ---
> drivers/net/tun.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 26f8635..ee328ba 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -452,7 +452,7 @@ static void __tun_detach(struct tun_file *tfile, bool
> clean)
>
> --tun->numqueues;
> if (clean) {
> - rcu_assign_pointer(tfile->tun, NULL);
> + RCU_INIT_POINTER(tfile->tun, NULL);
> sock_put(&tfile->sk);
> } else
> tun_disable_queue(tun, tfile);
> @@ -499,12 +499,12 @@ static void tun_detach_all(struct net_device *dev)
> tfile = rtnl_dereference(tun->tfiles[i]);
> BUG_ON(!tfile);
> wake_up_all(&tfile->wq.wait);
> - rcu_assign_pointer(tfile->tun, NULL);
> + RCU_INIT_POINTER(tfile->tun, NULL);
> --tun->numqueues;
> }
> list_for_each_entry(tfile, &tun->disabled, next) {
> wake_up_all(&tfile->wq.wait);
> - rcu_assign_pointer(tfile->tun, NULL);
> + RCU_INIT_POINTER(tfile->tun, NULL);
> }
> BUG_ON(tun->numqueues != 0);
>
> @@ -2194,7 +2194,7 @@ static int tun_chr_open(struct inode *inode, struct
> file * file)
> &tun_proto);
> if (!tfile)
> return -ENOMEM;
> - rcu_assign_pointer(tfile->tun, NULL);
> + RCU_INIT_POINTER(tfile->tun, NULL);
> tfile->net = get_net(current->nsproxy->net_ns);
> tfile->flags = 0;
> tfile->ifindex = 0;
> --
> 1.7.9.5
--->
The rcu_assign_pointer() ensures that the initialization of a structure
is carried out before storing a pointer to that structure.
In the case of the NULL pointer, there is no structure to initialize,
so we can safely drop smp_wmb in this case.
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
--
Lightly tested.
v is evaluated twice here but that should be ok since this
only happens when v is a constant, so evaluating it should
have no side effects.
Paul, what do you think?
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 72bf3a0..d33c9ec 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -587,7 +587,8 @@ static inline void rcu_preempt_sleep_check(void)
*/
#define rcu_assign_pointer(p, v) \
do { \
- smp_wmb(); \
+ if (!__builtin_constant_p(v) || (v)) \
+ smp_wmb(); \
ACCESS_ONCE(p) = RCU_INITIALIZER(v); \
} while (0)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
