LinuxLists.cc - [PATCH tip/core/rcu 0/9] Replace rcu_swap

2019-10-03 01:43:02

Subject: [PATCH tip/core/rcu 0/9] Replace rcu_swap_protected() for v5.5

Hello!

This series replaces uses of rcu_swap_protected() with rcu_replace().
A later patch will remove rcu_swap_protected().

1. Upgrade rcu_swap_protected() to rcu_replace().

2-9. Replace calls to rcu_swap_protected() with rcu_replace().

Thanx, Paul

------------------------------------------------------------------------

arch/x86/kvm/pmu.c | 4 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/scsi/scsi.c | 4 ++--
drivers/scsi/scsi_sysfs.c | 8 ++++----
fs/afs/vl_list.c | 4 ++--
include/linux/rcupdate.h | 18 ++++++++++++++++++
kernel/bpf/cgroup.c | 4 ++--
net/core/dev.c | 4 ++--
net/core/sock_reuseport.c | 4 ++--
net/netfilter/nf_tables_api.c | 5 +++--
net/sched/act_api.c | 2 +-
net/sched/act_csum.c | 4 ++--
net/sched/act_ct.c | 2 +-
net/sched/act_ctinfo.c | 4 ++--
net/sched/act_ife.c | 2 +-
net/sched/act_mirred.c | 4 ++--
net/sched/act_mpls.c | 2 +-
net/sched/act_police.c | 6 +++---
net/sched/act_skbedit.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 2 +-
21 files changed, 56 insertions(+), 37 deletions(-)

2019-10-03 01:44:07

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 7/9] net/core: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:44:11

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 6/9] bpf/cgroup: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:45:00

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 9/9] net/sched: Replace rcu_swap_protected() with rcu_replace()

From: "Paul E. McKenney" <[email protected]>

This commit replaces the use of rcu_swap_protected() with the more
intuitively appealing rcu_replace() as a step towards removing
rcu_swap_protected().

Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
Reported-by: Linus Torvalds <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Jamal Hadi Salim <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
net/sched/act_api.c | 2 +-
net/sched/act_csum.c | 4 ++--
net/sched/act_ct.c | 2 +-
net/sched/act_ctinfo.c | 4 ++--
net/sched/act_ife.c | 2 +-
net/sched/act_mirred.c | 4 ++--
net/sched/act_mpls.c | 2 +-
net/sched/act_police.c | 6 +++---
net/sched/act_skbedit.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 2 +-
11 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2558f00..1ab810c 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -88,7 +88,7 @@ struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *goto_chain)
{
a->tcfa_action = action;
- rcu_swap_protected(a->goto_chain, goto_chain, 1);
+ goto_chain = rcu_replace(a->goto_chain, goto_chain, 1);
return goto_chain;
}
EXPORT_SYMBOL(tcf_action_set_ctrlact);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index d3cfad8..ced5fe6 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -101,8 +101,8 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(p->params, params_new,
- lockdep_is_held(&p->tcf_lock));
+ params_new = rcu_replace(p->params, params_new,
+ lockdep_is_held(&p->tcf_lock));
spin_unlock_bh(&p->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index fcc4602..500e874 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -722,7 +722,7 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&c->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(c->params, params, lockdep_is_held(&c->tcf_lock));
+ params = rcu_replace(c->params, params, lockdep_is_held(&c->tcf_lock));
spin_unlock_bh(&c->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 0dbcfd1..e6ea270 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -257,8 +257,8 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&ci->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
- rcu_swap_protected(ci->params, cp_new,
- lockdep_is_held(&ci->tcf_lock));
+ cp_new = rcu_replace(ci->params, cp_new,
+ lockdep_is_held(&ci->tcf_lock));
spin_unlock_bh(&ci->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3a31e24..a6a60b8 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -594,7 +594,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&ife->tcf_lock);
/* protected by tcf_lock when modifying existing action */
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(ife->params, p, 1);
+ p = rcu_replace(ife->params, p, 1);

if (exists)
spin_unlock_bh(&ife->tcf_lock);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 9ce073a..1ed5d7e 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -178,8 +178,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
goto put_chain;
}
mac_header_xmit = dev_is_mac_header_xmit(dev);
- rcu_swap_protected(m->tcfm_dev, dev,
- lockdep_is_held(&m->tcf_lock));
+ dev = rcu_replace(m->tcfm_dev, dev,
+ lockdep_is_held(&m->tcf_lock));
if (dev)
dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index e168df0..cea8771 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -258,7 +258,7 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&m->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
+ p = rcu_replace(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
spin_unlock_bh(&m->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 89c04c5..02a4bc9c 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -191,9 +191,9 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
police->tcfp_ptoks = new->tcfp_mtu_ptoks;
spin_unlock_bh(&police->tcfp_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(police->params,
- new,
- lockdep_is_held(&police->tcf_lock));
+ new = rcu_replace(police->params,
+ new,
+ lockdep_is_held(&police->tcf_lock));
spin_unlock_bh(&police->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 6a8d333..6c4bd47 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -206,8 +206,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&d->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(d->params, params_new,
- lockdep_is_held(&d->tcf_lock));
+ params_new = rcu_replace(d->params, params_new,
+ lockdep_is_held(&d->tcf_lock));
spin_unlock_bh(&d->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 2f83a79..7130da8 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -381,8 +381,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&t->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(t->params, params_new,
- lockdep_is_held(&t->tcf_lock));
+ params_new = rcu_replace(t->params, params_new,
+ lockdep_is_held(&t->tcf_lock));
spin_unlock_bh(&t->tcf_lock);
tunnel_key_release_params(params_new);
if (goto_ch)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 08aaf71..402c9ea 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -220,7 +220,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&v->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
+ p = rcu_replace(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
spin_unlock_bh(&v->tcf_lock);

if (goto_ch)
--
2.9.5

2019-10-03 01:45:20

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 4/9] drivers/scsi: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:45:49

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 3/9] drivers/gpu: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:45:51

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 2/9] x86/kvm/pmu: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:46:35

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 8/9] net/netfilter: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:47:07

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 5/9] fs/afs: Replace rcu_swap_protected() with rcu_replace()

2019-10-03 01:47:39

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

From: "Paul E. McKenney" <[email protected]>

Although the rcu_swap_protected() macro follows the example of swap(),
the interactions with RCU make its update of its argument somewhat
counter-intuitive. This commit therefore introduces an rcu_replace()
that returns the old value of the RCU pointer instead of doing the
argument update. Once all the uses of rcu_swap_protected() are updated
to instead use rcu_replace(), rcu_swap_protected() will be removed.

Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
Reported-by: Linus Torvalds <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Shane M Seymour <[email protected]>
Cc: Martin K. Petersen <[email protected]>
---
include/linux/rcupdate.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 75a2ede..3b73287 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -383,6 +383,24 @@ do { \
} while (0)

/**
+ * rcu_replace() - replace an RCU pointer, returning its old value
+ * @rcu_ptr: RCU pointer, whose old value is returned
+ * @ptr: regular pointer
+ * @c: the lockdep conditions under which the dereference will take place
+ *
+ * Perform a replacement, where @rcu_ptr is an RCU-annotated
+ * pointer and @c is the lockdep argument that is passed to the
+ * rcu_dereference_protected() call used to read that pointer. The old
+ * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
+ */
+#define rcu_replace(rcu_ptr, ptr, c) \
+({ \
+ typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
+ rcu_assign_pointer((rcu_ptr), (ptr)); \
+ __tmp; \
+})
+
+/**
* rcu_swap_protected() - swap an RCU and a regular pointer
* @rcu_ptr: RCU pointer
* @ptr: regular pointer
--
2.9.5

2019-10-03 08:40:17

by David Howells

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

[email protected] wrote:

> +#define rcu_replace(rcu_ptr, ptr, c) \
> +({ \
> + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> + rcu_assign_pointer((rcu_ptr), (ptr)); \
> + __tmp; \
> +})

Does it make sense to actually use xchg() if that's supported by the arch?

David

2019-10-03 08:42:05

by David Howells

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 5/9] fs/afs: Replace rcu_swap_protected() with rcu_replace()

[email protected] wrote:

> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace() as a step towards removing
> rcu_swap_protected().

Yay!

> Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>

Acked-by: David Howells <[email protected]>

2019-10-03 10:19:07

by Paolo Bonzini

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 2/9] x86/kvm/pmu: Replace rcu_swap_protected() with rcu_replace()

On 03/10/19 03:43, [email protected] wrote:
> From: "Paul E. McKenney" <[email protected]>
>
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace() as a step towards removing
> rcu_swap_protected().
>
> Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: "Radim Krčmář" <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> ---
> arch/x86/kvm/pmu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 46875bb..4c37266 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -416,8 +416,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
> *filter = tmp;
>
> mutex_lock(&kvm->lock);
> - rcu_swap_protected(kvm->arch.pmu_event_filter, filter,
> - mutex_is_locked(&kvm->lock));
> + filter = rcu_replace(kvm->arch.pmu_event_filter, filter,
> + mutex_is_locked(&kvm->lock));
> mutex_unlock(&kvm->lock);
>
> synchronize_srcu_expedited(&kvm->srcu);
>

Should go without saying, but

Acked-by: Paolo Bonzini <[email protected]>

Paolo

2019-10-03 14:31:42

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, 03 Oct 2019 09:39:17 +0100
David Howells <[email protected]> wrote:

> [email protected] wrote:
>
> > +#define rcu_replace(rcu_ptr, ptr, c) \
> > +({ \
> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > + __tmp; \
> > +})
>
> Does it make sense to actually use xchg() if that's supported by the arch?
>

Hmm, is there really any arch that doesn't support xchg()? It would be
very hard to do any kind of atomic operations without it.

cmpxchg() is the one that I understand is optional by the arch.

-- Steve

2019-10-03 14:39:07

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 09:08:50AM -0400, Steven Rostedt wrote:
> On Thu, 03 Oct 2019 09:39:17 +0100
> David Howells <[email protected]> wrote:
>
> > [email protected] wrote:
> >
> > > +#define rcu_replace(rcu_ptr, ptr, c) \
> > > +({ \
> > > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > > + __tmp; \
> > > +})
> >
> > Does it make sense to actually use xchg() if that's supported by the arch?

Historically, xchg() has been quite a bit slower than a pair of assignment
statements, in part due to the strong memory ordering guaranteed by
xchg(). Has that changed? If so, then, agreed, it might make sense to
use xchg().

> Hmm, is there really any arch that doesn't support xchg()? It would be
> very hard to do any kind of atomic operations without it.
>
> cmpxchg() is the one that I understand is optional by the arch.

To your point, even the old Sequent Symmetry platforms supported xchg()
back in the late 1980s and early 1990s, but only the later versions
(with 80486 and later) supported cmpxchg().

Thanx, Paul

2019-10-03 14:41:43

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 06:33:15AM -0700, Paul E. McKenney wrote:
> On Thu, Oct 03, 2019 at 09:08:50AM -0400, Steven Rostedt wrote:
> > On Thu, 03 Oct 2019 09:39:17 +0100
> > David Howells <[email protected]> wrote:
> >
> > > [email protected] wrote:
> > >
> > > > +#define rcu_replace(rcu_ptr, ptr, c) \
> > > > +({ \
> > > > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > > > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > > > + __tmp; \
> > > > +})
> > >
> > > Does it make sense to actually use xchg() if that's supported by the arch?
>
> Historically, xchg() has been quite a bit slower than a pair of assignment
> statements, in part due to the strong memory ordering guaranteed by
> xchg(). Has that changed? If so, then, agreed, it might make sense to
> use xchg().

Nope, still the case. xchg() is an atomic op with full ordering.

2019-10-03 14:47:34

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 03:41:31PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 03, 2019 at 06:33:15AM -0700, Paul E. McKenney wrote:
> > On Thu, Oct 03, 2019 at 09:08:50AM -0400, Steven Rostedt wrote:
> > > On Thu, 03 Oct 2019 09:39:17 +0100
> > > David Howells <[email protected]> wrote:
> > >
> > > > [email protected] wrote:
> > > >
> > > > > +#define rcu_replace(rcu_ptr, ptr, c) \
> > > > > +({ \
> > > > > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > > > > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > > > > + __tmp; \
> > > > > +})
> > > >
> > > > Does it make sense to actually use xchg() if that's supported by the arch?
> >
> > Historically, xchg() has been quite a bit slower than a pair of assignment
> > statements, in part due to the strong memory ordering guaranteed by
> > xchg(). Has that changed? If so, then, agreed, it might make sense to
> > use xchg().
>
> Nope, still the case. xchg() is an atomic op with full ordering.

OK, let's stick with the pair of assignments, then. ;-)

Thanx, Paul

2019-10-03 14:47:54

by Joel Fernandes

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 06:33:15AM -0700, Paul E. McKenney wrote:
> On Thu, Oct 03, 2019 at 09:08:50AM -0400, Steven Rostedt wrote:
> > On Thu, 03 Oct 2019 09:39:17 +0100
> > David Howells <[email protected]> wrote:
> >
> > > [email protected] wrote:
> > >
> > > > +#define rcu_replace(rcu_ptr, ptr, c) \
> > > > +({ \
> > > > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > > > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > > > + __tmp; \
> > > > +})
> > >
> > > Does it make sense to actually use xchg() if that's supported by the arch?
>
> Historically, xchg() has been quite a bit slower than a pair of assignment
> statements, in part due to the strong memory ordering guaranteed by
> xchg(). Has that changed? If so, then, agreed, it might make sense to
> use xchg().

For the kfree_rcu() performance testing I was working on recently, replacing
xchg() with a pair of assignment statements in the code being tested provided
a great performance increase (on x86).

thanks,

- Joel

2019-10-03 16:54:19

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 12:35:30PM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 2, 2019, at 9:43 PM, paulmck [email protected] wrote:
>
> > From: "Paul E. McKenney" <[email protected]>
> >
> > Although the rcu_swap_protected() macro follows the example of swap(),
> > the interactions with RCU make its update of its argument somewhat
> > counter-intuitive. This commit therefore introduces an rcu_replace()
> > that returns the old value of the RCU pointer instead of doing the
> > argument update. Once all the uses of rcu_swap_protected() are updated
> > to instead use rcu_replace(), rcu_swap_protected() will be removed.
>
> We expose the rcu_xchg_pointer() API in liburcu (Userspace RCU) project.
> Any reason for not going that way and keep the kernel and user-space RCU
> APIs alike ?
>
> It's of course fine if they diverge, but we might want to at least consider
> if using the same API name would be OK.

Different semantics. An rcu_xchg_pointer() allows concurrent updates,
and rcu_replace() does not.

But yes, if someone needs the concurrent updates, rcu_xchg_pointer()
would certainly be my choice for the name.

Thanx, Paul

> Thanks,
>
> Mathieu
>
>
> >
> > Link:
> > https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Bart Van Assche <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Hannes Reinecke <[email protected]>
> > Cc: Johannes Thumshirn <[email protected]>
> > Cc: Shane M Seymour <[email protected]>
> > Cc: Martin K. Petersen <[email protected]>
> > ---
> > include/linux/rcupdate.h | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 75a2ede..3b73287 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -383,6 +383,24 @@ do { \
> > } while (0)
> >
> > /**
> > + * rcu_replace() - replace an RCU pointer, returning its old value
> > + * @rcu_ptr: RCU pointer, whose old value is returned
> > + * @ptr: regular pointer
> > + * @c: the lockdep conditions under which the dereference will take place
> > + *
> > + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> > + * pointer and @c is the lockdep argument that is passed to the
> > + * rcu_dereference_protected() call used to read that pointer. The old
> > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> > + */
> > +#define rcu_replace(rcu_ptr, ptr, c) \
> > +({ \
> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> > + __tmp; \
> > +})
> > +
> > +/**
> > * rcu_swap_protected() - swap an RCU and a regular pointer
> > * @rcu_ptr: RCU pointer
> > * @ptr: regular pointer
> > --
> > 2.9.5
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

2019-10-03 17:10:08

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

----- On Oct 2, 2019, at 9:43 PM, paulmck [email protected] wrote:

> From: "Paul E. McKenney" <[email protected]>
>
> Although the rcu_swap_protected() macro follows the example of swap(),
> the interactions with RCU make its update of its argument somewhat
> counter-intuitive. This commit therefore introduces an rcu_replace()
> that returns the old value of the RCU pointer instead of doing the
> argument update. Once all the uses of rcu_swap_protected() are updated
> to instead use rcu_replace(), rcu_swap_protected() will be removed.

We expose the rcu_xchg_pointer() API in liburcu (Userspace RCU) project.
Any reason for not going that way and keep the kernel and user-space RCU
APIs alike ?

It's of course fine if they diverge, but we might want to at least consider
if using the same API name would be OK.

Thanks,

Mathieu

>
> Link:
> https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Bart Van Assche <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Hannes Reinecke <[email protected]>
> Cc: Johannes Thumshirn <[email protected]>
> Cc: Shane M Seymour <[email protected]>
> Cc: Martin K. Petersen <[email protected]>
> ---
> include/linux/rcupdate.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 75a2ede..3b73287 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -383,6 +383,24 @@ do { \
> } while (0)
>
> /**
> + * rcu_replace() - replace an RCU pointer, returning its old value
> + * @rcu_ptr: RCU pointer, whose old value is returned
> + * @ptr: regular pointer
> + * @c: the lockdep conditions under which the dereference will take place
> + *
> + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> + * pointer and @c is the lockdep argument that is passed to the
> + * rcu_dereference_protected() call used to read that pointer. The old
> + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> + */
> +#define rcu_replace(rcu_ptr, ptr, c) \
> +({ \
> + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> + rcu_assign_pointer((rcu_ptr), (ptr)); \
> + __tmp; \
> +})
> +
> +/**
> * rcu_swap_protected() - swap an RCU and a regular pointer
> * @rcu_ptr: RCU pointer
> * @ptr: regular pointer
> --
> 2.9.5

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2019-10-03 17:13:42

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

----- On Oct 3, 2019, at 9:08 AM, rostedt [email protected] wrote:

> On Thu, 03 Oct 2019 09:39:17 +0100
> David Howells <[email protected]> wrote:
>
>> [email protected] wrote:
>>
>> > +#define rcu_replace(rcu_ptr, ptr, c) \
>> > +({ \
>> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
>> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
>> > + __tmp; \
>> > +})
>>
>> Does it make sense to actually use xchg() if that's supported by the arch?
>>
>
> Hmm, is there really any arch that doesn't support xchg()? It would be
> very hard to do any kind of atomic operations without it.
>
> cmpxchg() is the one that I understand is optional by the arch.

I remember going through all kernel arch headers myself and introduce
irq-off-based xchg and cmpxchg generic implementations for all UP
architectures lacking cmpxchg/xchg instructions.

We might want to consider simply using xchg() here.

FWIW, the API exposed by the Userspace RCU project (liburcu) for this
is:

rcu_xchg_pointer()

Thanks,

Mathieu

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2019-10-03 17:43:09

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

----- On Oct 3, 2019, at 12:52 PM, paulmck [email protected] wrote:

> On Thu, Oct 03, 2019 at 12:35:30PM -0400, Mathieu Desnoyers wrote:
>> ----- On Oct 2, 2019, at 9:43 PM, paulmck [email protected] wrote:
>>
>> > From: "Paul E. McKenney" <[email protected]>
>> >
>> > Although the rcu_swap_protected() macro follows the example of swap(),
>> > the interactions with RCU make its update of its argument somewhat
>> > counter-intuitive. This commit therefore introduces an rcu_replace()
>> > that returns the old value of the RCU pointer instead of doing the
>> > argument update. Once all the uses of rcu_swap_protected() are updated
>> > to instead use rcu_replace(), rcu_swap_protected() will be removed.
>>
>> We expose the rcu_xchg_pointer() API in liburcu (Userspace RCU) project.
>> Any reason for not going that way and keep the kernel and user-space RCU
>> APIs alike ?
>>
>> It's of course fine if they diverge, but we might want to at least consider
>> if using the same API name would be OK.
>
> Different semantics. An rcu_xchg_pointer() allows concurrent updates,
> and rcu_replace() does not.
>
> But yes, if someone needs the concurrent updates, rcu_xchg_pointer()
> would certainly be my choice for the name.

Then considering that its assignment counterpart is "rcu_assign_pointer()"
(and not "rcu_assign()"), would "rcu_replace_pointer()" be less ambiguous
about its intended use ?

Thanks,

Mathieu

>
> Thanx, Paul
>
>> Thanks,
>>
>> Mathieu
>>
>>
>> >
>> > Link:
>> > https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
>> > Reported-by: Linus Torvalds <[email protected]>
>> > Signed-off-by: Paul E. McKenney <[email protected]>
>> > Cc: Bart Van Assche <[email protected]>
>> > Cc: Christoph Hellwig <[email protected]>
>> > Cc: Hannes Reinecke <[email protected]>
>> > Cc: Johannes Thumshirn <[email protected]>
>> > Cc: Shane M Seymour <[email protected]>
>> > Cc: Martin K. Petersen <[email protected]>
>> > ---
>> > include/linux/rcupdate.h | 18 ++++++++++++++++++
>> > 1 file changed, 18 insertions(+)
>> >
>> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>> > index 75a2ede..3b73287 100644
>> > --- a/include/linux/rcupdate.h
>> > +++ b/include/linux/rcupdate.h
>> > @@ -383,6 +383,24 @@ do { \
>> > } while (0)
>> >
>> > /**
>> > + * rcu_replace() - replace an RCU pointer, returning its old value
>> > + * @rcu_ptr: RCU pointer, whose old value is returned
>> > + * @ptr: regular pointer
>> > + * @c: the lockdep conditions under which the dereference will take place
>> > + *
>> > + * Perform a replacement, where @rcu_ptr is an RCU-annotated
>> > + * pointer and @c is the lockdep argument that is passed to the
>> > + * rcu_dereference_protected() call used to read that pointer. The old
>> > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
>> > + */
>> > +#define rcu_replace(rcu_ptr, ptr, c) \
>> > +({ \
>> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
>> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
>> > + __tmp; \
>> > +})
>> > +
>> > +/**
>> > * rcu_swap_protected() - swap an RCU and a regular pointer
>> > * @rcu_ptr: RCU pointer
>> > * @ptr: regular pointer
>> > --
>> > 2.9.5
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2019-10-03 17:45:44

by Andrii Nakryiko

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 6/9] bpf/cgroup: Replace rcu_swap_protected() with rcu_replace()

On Wed, Oct 2, 2019 at 6:45 PM <[email protected]> wrote:
>
> From: "Paul E. McKenney" <[email protected]>
>
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace() as a step towards removing
> rcu_swap_protected().
>
> Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Martin KaFai Lau <[email protected]>
> Cc: Song Liu <[email protected]>
> Cc: Yonghong Song <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> ---

Acked-by: Andrii Nakryiko <[email protected]>

> kernel/bpf/cgroup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ddd8add..06a0657 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -180,8 +180,8 @@ static void activate_effective_progs(struct cgroup *cgrp,
> enum bpf_attach_type type,
> struct bpf_prog_array *old_array)
> {
> - rcu_swap_protected(cgrp->bpf.effective[type], old_array,
> - lockdep_is_held(&cgroup_mutex));
> + old_array = rcu_replace(cgrp->bpf.effective[type], old_array,
> + lockdep_is_held(&cgroup_mutex));
> /* free prog array after grace period, since __cgroup_bpf_run_*()
> * might be still walking the array
> */
> --
> 2.9.5
>

2019-10-03 18:58:10

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 12:31:38PM -0400, Mathieu Desnoyers wrote:
> We might want to consider simply using xchg() here.

As stated elsewhere, xchg() is an atomic op with full ordering, IOW it
is stupid expensive for what needs to be done.

2019-10-03 19:15:38

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

On Thu, Oct 03, 2019 at 01:21:14PM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 3, 2019, at 12:52 PM, paulmck [email protected] wrote:
>
> > On Thu, Oct 03, 2019 at 12:35:30PM -0400, Mathieu Desnoyers wrote:
> >> ----- On Oct 2, 2019, at 9:43 PM, paulmck [email protected] wrote:
> >>
> >> > From: "Paul E. McKenney" <[email protected]>
> >> >
> >> > Although the rcu_swap_protected() macro follows the example of swap(),
> >> > the interactions with RCU make its update of its argument somewhat
> >> > counter-intuitive. This commit therefore introduces an rcu_replace()
> >> > that returns the old value of the RCU pointer instead of doing the
> >> > argument update. Once all the uses of rcu_swap_protected() are updated
> >> > to instead use rcu_replace(), rcu_swap_protected() will be removed.
> >>
> >> We expose the rcu_xchg_pointer() API in liburcu (Userspace RCU) project.
> >> Any reason for not going that way and keep the kernel and user-space RCU
> >> APIs alike ?
> >>
> >> It's of course fine if they diverge, but we might want to at least consider
> >> if using the same API name would be OK.
> >
> > Different semantics. An rcu_xchg_pointer() allows concurrent updates,
> > and rcu_replace() does not.
> >
> > But yes, if someone needs the concurrent updates, rcu_xchg_pointer()
> > would certainly be my choice for the name.
>
> Then considering that its assignment counterpart is "rcu_assign_pointer()"
> (and not "rcu_assign()"), would "rcu_replace_pointer()" be less ambiguous
> about its intended use ?

The sequence was rcu_swap_protected() -> rcu_swap() -> rcu_replace().
Because that rcu_replace(), unlike rcu_swap_protected(), returns a
value, the shorter name is valuable.

Maybe we should have used rcu_assign() instead of rcu_assign_pointer(),
but there is no point in that sort of change at this late date!

Thanx, Paul

> Thanks,
>
> Mathieu
>
>
> >
> > Thanx, Paul
> >
> >> Thanks,
> >>
> >> Mathieu
> >>
> >>
> >> >
> >> > Link:
> >> > https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> >> > Reported-by: Linus Torvalds <[email protected]>
> >> > Signed-off-by: Paul E. McKenney <[email protected]>
> >> > Cc: Bart Van Assche <[email protected]>
> >> > Cc: Christoph Hellwig <[email protected]>
> >> > Cc: Hannes Reinecke <[email protected]>
> >> > Cc: Johannes Thumshirn <[email protected]>
> >> > Cc: Shane M Seymour <[email protected]>
> >> > Cc: Martin K. Petersen <[email protected]>
> >> > ---
> >> > include/linux/rcupdate.h | 18 ++++++++++++++++++
> >> > 1 file changed, 18 insertions(+)
> >> >
> >> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> >> > index 75a2ede..3b73287 100644
> >> > --- a/include/linux/rcupdate.h
> >> > +++ b/include/linux/rcupdate.h
> >> > @@ -383,6 +383,24 @@ do { \
> >> > } while (0)
> >> >
> >> > /**
> >> > + * rcu_replace() - replace an RCU pointer, returning its old value
> >> > + * @rcu_ptr: RCU pointer, whose old value is returned
> >> > + * @ptr: regular pointer
> >> > + * @c: the lockdep conditions under which the dereference will take place
> >> > + *
> >> > + * Perform a replacement, where @rcu_ptr is an RCU-annotated
> >> > + * pointer and @c is the lockdep argument that is passed to the
> >> > + * rcu_dereference_protected() call used to read that pointer. The old
> >> > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
> >> > + */
> >> > +#define rcu_replace(rcu_ptr, ptr, c) \
> >> > +({ \
> >> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
> >> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
> >> > + __tmp; \
> >> > +})
> >> > +
> >> > +/**
> >> > * rcu_swap_protected() - swap an RCU and a regular pointer
> >> > * @rcu_ptr: RCU pointer
> >> > * @ptr: regular pointer
> >> > --
> >> > 2.9.5
> >>
> >> --
> >> Mathieu Desnoyers
> >> EfficiOS Inc.
> > > http://www.efficios.com
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

2019-10-03 19:17:35

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 1/9] rcu: Upgrade rcu_swap_protected() to rcu_replace()

----- On Oct 3, 2019, at 3:09 PM, paulmck [email protected] wrote:

> On Thu, Oct 03, 2019 at 01:21:14PM -0400, Mathieu Desnoyers wrote:
>> ----- On Oct 3, 2019, at 12:52 PM, paulmck [email protected] wrote:
>>
>> > On Thu, Oct 03, 2019 at 12:35:30PM -0400, Mathieu Desnoyers wrote:
>> >> ----- On Oct 2, 2019, at 9:43 PM, paulmck [email protected] wrote:
>> >>
>> >> > From: "Paul E. McKenney" <[email protected]>
>> >> >
>> >> > Although the rcu_swap_protected() macro follows the example of swap(),
>> >> > the interactions with RCU make its update of its argument somewhat
>> >> > counter-intuitive. This commit therefore introduces an rcu_replace()
>> >> > that returns the old value of the RCU pointer instead of doing the
>> >> > argument update. Once all the uses of rcu_swap_protected() are updated
>> >> > to instead use rcu_replace(), rcu_swap_protected() will be removed.
>> >>
>> >> We expose the rcu_xchg_pointer() API in liburcu (Userspace RCU) project.
>> >> Any reason for not going that way and keep the kernel and user-space RCU
>> >> APIs alike ?
>> >>
>> >> It's of course fine if they diverge, but we might want to at least consider
>> >> if using the same API name would be OK.
>> >
>> > Different semantics. An rcu_xchg_pointer() allows concurrent updates,
>> > and rcu_replace() does not.
>> >
>> > But yes, if someone needs the concurrent updates, rcu_xchg_pointer()
>> > would certainly be my choice for the name.
>>
>> Then considering that its assignment counterpart is "rcu_assign_pointer()"
>> (and not "rcu_assign()"), would "rcu_replace_pointer()" be less ambiguous
>> about its intended use ?
>
> The sequence was rcu_swap_protected() -> rcu_swap() -> rcu_replace().
> Because that rcu_replace(), unlike rcu_swap_protected(), returns a
> value, the shorter name is valuable.
>
> Maybe we should have used rcu_assign() instead of rcu_assign_pointer(),
> but there is no point in that sort of change at this late date!

I agree that having both the "rcu_" prefix and the "_pointer" suffix is
somewhat redundant. Indeed it's not a worthwhile change for a pre-existing
API, but it is welcome for a new API.

Thanks!

Mathieu

>
> Thanx, Paul
>
>> Thanks,
>>
>> Mathieu
>>
>>
>> >
>> > Thanx, Paul
>> >
>> >> Thanks,
>> >>
>> >> Mathieu
>> >>
>> >>
>> >> >
>> >> > Link:
>> >> > https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
>> >> > Reported-by: Linus Torvalds <[email protected]>
>> >> > Signed-off-by: Paul E. McKenney <[email protected]>
>> >> > Cc: Bart Van Assche <[email protected]>
>> >> > Cc: Christoph Hellwig <[email protected]>
>> >> > Cc: Hannes Reinecke <[email protected]>
>> >> > Cc: Johannes Thumshirn <[email protected]>
>> >> > Cc: Shane M Seymour <[email protected]>
>> >> > Cc: Martin K. Petersen <[email protected]>
>> >> > ---
>> >> > include/linux/rcupdate.h | 18 ++++++++++++++++++
>> >> > 1 file changed, 18 insertions(+)
>> >> >
>> >> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>> >> > index 75a2ede..3b73287 100644
>> >> > --- a/include/linux/rcupdate.h
>> >> > +++ b/include/linux/rcupdate.h
>> >> > @@ -383,6 +383,24 @@ do { \
>> >> > } while (0)
>> >> >
>> >> > /**
>> >> > + * rcu_replace() - replace an RCU pointer, returning its old value
>> >> > + * @rcu_ptr: RCU pointer, whose old value is returned
>> >> > + * @ptr: regular pointer
>> >> > + * @c: the lockdep conditions under which the dereference will take place
>> >> > + *
>> >> > + * Perform a replacement, where @rcu_ptr is an RCU-annotated
>> >> > + * pointer and @c is the lockdep argument that is passed to the
>> >> > + * rcu_dereference_protected() call used to read that pointer. The old
>> >> > + * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
>> >> > + */
>> >> > +#define rcu_replace(rcu_ptr, ptr, c) \
>> >> > +({ \
>> >> > + typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
>> >> > + rcu_assign_pointer((rcu_ptr), (ptr)); \
>> >> > + __tmp; \
>> >> > +})
>> >> > +
>> >> > +/**
>> >> > * rcu_swap_protected() - swap an RCU and a regular pointer
>> >> > * @rcu_ptr: RCU pointer
>> >> > * @ptr: regular pointer
>> >> > --
>> >> > 2.9.5
>> >>
>> >> --
>> >> Mathieu Desnoyers
>> >> EfficiOS Inc.
>> > > http://www.efficios.com
>>
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
> > http://www.efficios.com

--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

2019-10-03 21:02:04

by Song Liu

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 6/9] bpf/cgroup: Replace rcu_swap_protected() with rcu_replace()

On Thu, Oct 3, 2019 at 10:43 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Wed, Oct 2, 2019 at 6:45 PM <[email protected]> wrote:
> >
> > From: "Paul E. McKenney" <[email protected]>
> >
> > This commit replaces the use of rcu_swap_protected() with the more
> > intuitively appealing rcu_replace() as a step towards removing
> > rcu_swap_protected().
> >
> > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Daniel Borkmann <[email protected]>
> > Cc: Martin KaFai Lau <[email protected]>
> > Cc: Song Liu <[email protected]>
> > Cc: Yonghong Song <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > ---
>
> Acked-by: Andrii Nakryiko <[email protected]>

Acked-by: Song Liu <[email protected]>

2019-10-04 07:57:45

by Martin K. Petersen

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 4/9] drivers/scsi: Replace rcu_swap_protected() with rcu_replace()

Paul,

No objections from me.

> + vpd_pg80 = rcu_replace(sdev->vpd_pg80, vpd_pg80,
> + lockdep_is_held(&sdev->inquiry_mutex));
> + vpd_pg83 = rcu_replace(sdev->vpd_pg83, vpd_pg83,
> + lockdep_is_held(&sdev->inquiry_mutex));

Just a heads-up that we have added a couple of additional VPD pages so
my 5.5 tree will need additional calls to be updated to rcu_replace().

--
Martin K. Petersen Oracle Linux Engineering

2019-10-05 16:12:25

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 5/9] fs/afs: Replace rcu_swap_protected() with rcu_replace()

On Thu, Oct 03, 2019 at 09:38:14AM +0100, David Howells wrote:
> [email protected] wrote:
>
> > This commit replaces the use of rcu_swap_protected() with the more
> > intuitively appealing rcu_replace() as a step towards removing
> > rcu_swap_protected().
>
> Yay!
>
> > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: David Howells <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
>
> Acked-by: David Howells <[email protected]>

Applied, thank you David!

Thanx, Paul

2019-10-05 16:13:44

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 4/9] drivers/scsi: Replace rcu_swap_protected() with rcu_replace()

On Thu, Oct 03, 2019 at 10:09:31PM -0400, Martin K. Petersen wrote:
>
> Paul,
>
> No objections from me.

Thank you, Martin! I have applied your Acked-by, but please let me
know if that over-interprets your "No objections" above.

> > + vpd_pg80 = rcu_replace(sdev->vpd_pg80, vpd_pg80,
> > + lockdep_is_held(&sdev->inquiry_mutex));
> > + vpd_pg83 = rcu_replace(sdev->vpd_pg83, vpd_pg83,
> > + lockdep_is_held(&sdev->inquiry_mutex));
>
> Just a heads-up that we have added a couple of additional VPD pages so
> my 5.5 tree will need additional calls to be updated to rcu_replace().

I do not intend to actually remove rcu_swap_protected() until 5.6 for
exactly this sort of thing. My plan is to take another pass through
the tree after 5.5 comes out, and these will be caught at that time.

Does that work for you?

Thanx, Paul

2019-10-05 16:13:46

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 2/9] x86/kvm/pmu: Replace rcu_swap_protected() with rcu_replace()

On Thu, Oct 03, 2019 at 12:14:32PM +0200, Paolo Bonzini wrote:
> On 03/10/19 03:43, [email protected] wrote:
> > From: "Paul E. McKenney" <[email protected]>
> >
> > This commit replaces the use of rcu_swap_protected() with the more
> > intuitively appealing rcu_replace() as a step towards removing
> > rcu_swap_protected().
> >
> > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: "Radim Krčmář" <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > ---
> > arch/x86/kvm/pmu.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index 46875bb..4c37266 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -416,8 +416,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
> > *filter = tmp;
> >
> > mutex_lock(&kvm->lock);
> > - rcu_swap_protected(kvm->arch.pmu_event_filter, filter,
> > - mutex_is_locked(&kvm->lock));
> > + filter = rcu_replace(kvm->arch.pmu_event_filter, filter,
> > + mutex_is_locked(&kvm->lock));
> > mutex_unlock(&kvm->lock);
> >
> > synchronize_srcu_expedited(&kvm->srcu);
> >
>
> Should go without saying, but
>
> Acked-by: Paolo Bonzini <[email protected]>

It never goes without saying! ;-)

Applied, thank you!

Thanx, Paul

2019-10-05 16:16:04

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 6/9] bpf/cgroup: Replace rcu_swap_protected() with rcu_replace()

On Thu, Oct 03, 2019 at 01:58:13PM -0700, Song Liu wrote:
> On Thu, Oct 3, 2019 at 10:43 AM Andrii Nakryiko
> <[email protected]> wrote:
> >
> > On Wed, Oct 2, 2019 at 6:45 PM <[email protected]> wrote:
> > >
> > > From: "Paul E. McKenney" <[email protected]>
> > >
> > > This commit replaces the use of rcu_swap_protected() with the more
> > > intuitively appealing rcu_replace() as a step towards removing
> > > rcu_swap_protected().
> > >
> > > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > > Reported-by: Linus Torvalds <[email protected]>
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > Cc: Alexei Starovoitov <[email protected]>
> > > Cc: Daniel Borkmann <[email protected]>
> > > Cc: Martin KaFai Lau <[email protected]>
> > > Cc: Song Liu <[email protected]>
> > > Cc: Yonghong Song <[email protected]>
> > > Cc: <[email protected]>
> > > Cc: <[email protected]>
> > > ---
> >
> > Acked-by: Andrii Nakryiko <[email protected]>
>
> Acked-by: Song Liu <[email protected]>

Applied, thank you both!

Thanx, Paul

2019-10-08 14:17:20

by Pablo Neira Ayuso

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 8/9] net/netfilter: Replace rcu_swap_protected() with rcu_replace()

On Wed, Oct 02, 2019 at 06:43:09PM -0700, [email protected] wrote:
> From: "Paul E. McKenney" <[email protected]>
>
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace() as a step towards removing
> rcu_swap_protected().
>
> Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Pablo Neira Ayuso <[email protected]>
> Cc: Jozsef Kadlecsik <[email protected]>
> Cc: Florian Westphal <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>

Acked-by: Pablo Neira Ayuso <[email protected]>

> ---
> net/netfilter/nf_tables_api.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index d481f9b..8499baf 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1461,8 +1461,9 @@ static void nft_chain_stats_replace(struct nft_trans *trans)
> if (!nft_trans_chain_stats(trans))
> return;
>
> - rcu_swap_protected(chain->stats, nft_trans_chain_stats(trans),
> - lockdep_commit_lock_is_held(trans->ctx.net));
> + nft_trans_chain_stats(trans) =
> + rcu_replace(chain->stats, nft_trans_chain_stats(trans),
> + lockdep_commit_lock_is_held(trans->ctx.net));
>
> if (!nft_trans_chain_stats(trans))
> static_branch_inc(&nft_counters_enabled);
> --
> 2.9.5
>

2019-10-09 15:38:22

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 8/9] net/netfilter: Replace rcu_swap_protected() with rcu_replace()

On Tue, Oct 08, 2019 at 04:16:11PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Oct 02, 2019 at 06:43:09PM -0700, [email protected] wrote:
> > From: "Paul E. McKenney" <[email protected]>
> >
> > This commit replaces the use of rcu_swap_protected() with the more
> > intuitively appealing rcu_replace() as a step towards removing
> > rcu_swap_protected().
> >
> > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Pablo Neira Ayuso <[email protected]>
> > Cc: Jozsef Kadlecsik <[email protected]>
> > Cc: Florian Westphal <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
>
> Acked-by: Pablo Neira Ayuso <[email protected]>

Applied, thank you!

Thanx, Paul

> > ---
> > net/netfilter/nf_tables_api.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index d481f9b..8499baf 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -1461,8 +1461,9 @@ static void nft_chain_stats_replace(struct nft_trans *trans)
> > if (!nft_trans_chain_stats(trans))
> > return;
> >
> > - rcu_swap_protected(chain->stats, nft_trans_chain_stats(trans),
> > - lockdep_commit_lock_is_held(trans->ctx.net));
> > + nft_trans_chain_stats(trans) =
> > + rcu_replace(chain->stats, nft_trans_chain_stats(trans),
> > + lockdep_commit_lock_is_held(trans->ctx.net));
> >
> > if (!nft_trans_chain_stats(trans))
> > static_branch_inc(&nft_counters_enabled);
> > --
> > 2.9.5
> >

2019-10-10 02:38:23

by Martin K. Petersen

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 4/9] drivers/scsi: Replace rcu_swap_protected() with rcu_replace()

Paul,

> I do not intend to actually remove rcu_swap_protected() until 5.6 for
> exactly this sort of thing. My plan is to take another pass through
> the tree after 5.5 comes out, and these will be caught at that time.
>
> Does that work for you?

Yep, that's great. Thanks!

--
Martin K. Petersen Oracle Linux Engineering

2019-10-22 19:13:04

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH v2 tip/core/rcu 0/10] Replace rcu_swap_protected() for v5.5

Hello!

This series replaces uses of rcu_swap_protected() with rcu_replace().
A later patch will remove rcu_swap_protected().

1. Upgrade rcu_swap_protected() to rcu_replace().

2-10. Replace calls to rcu_swap_protected() with rcu_replace().

Changes from v1:

o Added security/safesetid patch.

o Changed the name from rcu_replace() to rcu_replace_pointer()
based on feedback from Ingo Molnar.

Thanx, Paul

------------------------------------------------------------------------

arch/x86/kvm/pmu.c | 4 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
drivers/scsi/scsi.c | 4 ++--
drivers/scsi/scsi_sysfs.c | 8 ++++----
fs/afs/vl_list.c | 4 ++--
include/linux/rcupdate.h | 18 ++++++++++++++++++
kernel/bpf/cgroup.c | 4 ++--
net/core/dev.c | 4 ++--
net/core/sock_reuseport.c | 4 ++--
net/netfilter/nf_tables_api.c | 5 +++--
net/sched/act_api.c | 2 +-
net/sched/act_csum.c | 4 ++--
net/sched/act_ct.c | 3 ++-
net/sched/act_ctinfo.c | 4 ++--
net/sched/act_ife.c | 2 +-
net/sched/act_mirred.c | 4 ++--
net/sched/act_mpls.c | 2 +-
net/sched/act_police.c | 6 +++---
net/sched/act_sample.c | 4 ++--
net/sched/act_skbedit.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 2 +-
security/safesetid/securityfs.c | 4 ++--
23 files changed, 61 insertions(+), 41 deletions(-)

2019-10-22 19:13:33

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 02/10] x86/kvm/pmu: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:13:37

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 03/10] drivers/gpu: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:13:46

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 04/10] drivers/scsi: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:13:56

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 09/10] net/sched: Replace rcu_swap_protected() with rcu_replace()

From: "Paul E. McKenney" <[email protected]>

This commit replaces the use of rcu_swap_protected() with the more
intuitively appealing rcu_replace() as a step towards removing
rcu_swap_protected().

Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
Reported-by: Linus Torvalds <[email protected]>
[ paulmck: From rcu_replace() to rcu_replace_pointer() per Ingo Molnar. ]
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Jamal Hadi Salim <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
net/sched/act_api.c | 2 +-
net/sched/act_csum.c | 4 ++--
net/sched/act_ct.c | 3 ++-
net/sched/act_ctinfo.c | 4 ++--
net/sched/act_ife.c | 2 +-
net/sched/act_mirred.c | 4 ++--
net/sched/act_mpls.c | 2 +-
net/sched/act_police.c | 6 +++---
net/sched/act_sample.c | 4 ++--
net/sched/act_skbedit.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 2 +-
12 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2558f00..3d51573 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -88,7 +88,7 @@ struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *goto_chain)
{
a->tcfa_action = action;
- rcu_swap_protected(a->goto_chain, goto_chain, 1);
+ goto_chain = rcu_replace_pointer(a->goto_chain, goto_chain, 1);
return goto_chain;
}
EXPORT_SYMBOL(tcf_action_set_ctrlact);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index d3cfad8..87dddba 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -101,8 +101,8 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(p->params, params_new,
- lockdep_is_held(&p->tcf_lock));
+ params_new = rcu_replace_pointer(p->params, params_new,
+ lockdep_is_held(&p->tcf_lock));
spin_unlock_bh(&p->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index fcc4602..2d5ab23 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -722,7 +722,8 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&c->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(c->params, params, lockdep_is_held(&c->tcf_lock));
+ params = rcu_replace_pointer(c->params, params,
+ lockdep_is_held(&c->tcf_lock));
spin_unlock_bh(&c->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 0dbcfd1..c599818 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -257,8 +257,8 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&ci->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
- rcu_swap_protected(ci->params, cp_new,
- lockdep_is_held(&ci->tcf_lock));
+ cp_new = rcu_replace_pointer(ci->params, cp_new,
+ lockdep_is_held(&ci->tcf_lock));
spin_unlock_bh(&ci->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3a31e24..2ea2e16 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -594,7 +594,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
spin_lock_bh(&ife->tcf_lock);
/* protected by tcf_lock when modifying existing action */
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(ife->params, p, 1);
+ p = rcu_replace_pointer(ife->params, p, 1);

if (exists)
spin_unlock_bh(&ife->tcf_lock);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 9ce073a..0c2f737 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -178,8 +178,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
goto put_chain;
}
mac_header_xmit = dev_is_mac_header_xmit(dev);
- rcu_swap_protected(m->tcfm_dev, dev,
- lockdep_is_held(&m->tcf_lock));
+ dev = rcu_replace_pointer(m->tcfm_dev, dev,
+ lockdep_is_held(&m->tcf_lock));
if (dev)
dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index e168df0..5b3031c 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -258,7 +258,7 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&m->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
+ p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
spin_unlock_bh(&m->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 89c04c5..caa91cf 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -191,9 +191,9 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
police->tcfp_ptoks = new->tcfp_mtu_ptoks;
spin_unlock_bh(&police->tcfp_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(police->params,
- new,
- lockdep_is_held(&police->tcf_lock));
+ new = rcu_replace_pointer(police->params,
+ new,
+ lockdep_is_held(&police->tcf_lock));
spin_unlock_bh(&police->tcf_lock);

if (goto_ch)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 514456a..4deeaf2 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -102,8 +102,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
s->rate = rate;
s->psample_group_num = psample_group_num;
- rcu_swap_protected(s->psample_group, psample_group,
- lockdep_is_held(&s->tcf_lock));
+ psample_group = rcu_replace_pointer(s->psample_group, psample_group,
+ lockdep_is_held(&s->tcf_lock));

if (tb[TCA_SAMPLE_TRUNC_SIZE]) {
s->truncate = true;
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 6a8d333..c38cc39 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -206,8 +206,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&d->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(d->params, params_new,
- lockdep_is_held(&d->tcf_lock));
+ params_new = rcu_replace_pointer(d->params, params_new,
+ lockdep_is_held(&d->tcf_lock));
spin_unlock_bh(&d->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 2f83a79..20d7ca4 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -381,8 +381,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&t->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(t->params, params_new,
- lockdep_is_held(&t->tcf_lock));
+ params_new = rcu_replace_pointer(t->params, params_new,
+ lockdep_is_held(&t->tcf_lock));
spin_unlock_bh(&t->tcf_lock);
tunnel_key_release_params(params_new);
if (goto_ch)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 08aaf71..7aca1f0 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -220,7 +220,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,

spin_lock_bh(&v->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
- rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
+ p = rcu_replace_pointer(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
spin_unlock_bh(&v->tcf_lock);

if (goto_ch)
--
2.9.5

2019-10-22 19:14:18

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 10/10] security/safesetid: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:14:48

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 07/10] net/core: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:15:10

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 05/10] fs/afs: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:15:52

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 06/10] bpf/cgroup: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:16:18

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 08/10] net/netfilter: Replace rcu_swap_protected() with rcu_replace()

2019-10-22 19:17:55

by Paul E. McKenney

[permalink] [raw]

Subject: [PATCH tip/core/rcu 01/10] rcu: Upgrade rcu_swap_protected() to rcu_replace()

From: "Paul E. McKenney" <[email protected]>

Although the rcu_swap_protected() macro follows the example of swap(),
the interactions with RCU make its update of its argument somewhat
counter-intuitive. This commit therefore introduces an rcu_replace()
that returns the old value of the RCU pointer instead of doing the
argument update. Once all the uses of rcu_swap_protected() are updated
to instead use rcu_replace(), rcu_swap_protected() will be removed.

Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
Reported-by: Linus Torvalds <[email protected]>
[ paulmck: From rcu_replace() to rcu_replace_pointer() per Ingo Molnar. ]
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Shane M Seymour <[email protected]>
Cc: Martin K. Petersen <[email protected]>
---
include/linux/rcupdate.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 75a2ede..185dd97 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -383,6 +383,24 @@ do { \
} while (0)

/**
+ * rcu_replace_pointer() - replace an RCU pointer, returning its old value
+ * @rcu_ptr: RCU pointer, whose old value is returned
+ * @ptr: regular pointer
+ * @c: the lockdep conditions under which the dereference will take place
+ *
+ * Perform a replacement, where @rcu_ptr is an RCU-annotated
+ * pointer and @c is the lockdep argument that is passed to the
+ * rcu_dereference_protected() call used to read that pointer. The old
+ * value of @rcu_ptr is returned, and @rcu_ptr is set to @ptr.
+ */
+#define rcu_replace_pointer(rcu_ptr, ptr, c) \
+({ \
+ typeof(ptr) __tmp = rcu_dereference_protected((rcu_ptr), (c)); \
+ rcu_assign_pointer((rcu_ptr), (ptr)); \
+ __tmp; \
+})
+
+/**
* rcu_swap_protected() - swap an RCU and a regular pointer
* @rcu_ptr: RCU pointer
* @ptr: regular pointer
--
2.9.5

2019-10-28 20:28:33

by Joonas Lahtinen

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 03/10] drivers/gpu: Replace rcu_swap_protected() with rcu_replace()

Quoting [email protected] (2019-10-22 22:12:08)
> From: "Paul E. McKenney" <[email protected]>
>
> This commit replaces the use of rcu_swap_protected() with the more
> intuitively appealing rcu_replace() as a step towards removing
> rcu_swap_protected().
>
> Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> Reported-by: Linus Torvalds <[email protected]>
> [ paulmck: From rcu_replace() to rcu_replace_pointer() per Ingo Molnar. ]
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Jani Nikula <[email protected]>
> Cc: Joonas Lahtinen <[email protected]>
> Cc: Rodrigo Vivi <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Chris Wilson <[email protected]>
> Cc: Tvrtko Ursulin <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>

"drm/i915:" preferred as the subject prefix for increased specificity.

Let me know which tree you end up merging with.

Reviewed-by: Joonas Lahtinen <[email protected]>

Regards, Joonas

2019-10-29 02:12:56

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [PATCH tip/core/rcu 03/10] drivers/gpu: Replace rcu_swap_protected() with rcu_replace()

On Mon, Oct 28, 2019 at 02:57:26PM +0200, Joonas Lahtinen wrote:
> Quoting [email protected] (2019-10-22 22:12:08)
> > From: "Paul E. McKenney" <[email protected]>
> >
> > This commit replaces the use of rcu_swap_protected() with the more
> > intuitively appealing rcu_replace() as a step towards removing
> > rcu_swap_protected().
> >
> > Link: https://lore.kernel.org/lkml/CAHk-=wiAsJLw1egFEE=Z7-GGtM6wcvtyytXZA1+BHqta4gg6Hw@mail.gmail.com/
> > Reported-by: Linus Torvalds <[email protected]>
> > [ paulmck: From rcu_replace() to rcu_replace_pointer() per Ingo Molnar. ]
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Jani Nikula <[email protected]>
> > Cc: Joonas Lahtinen <[email protected]>
> > Cc: Rodrigo Vivi <[email protected]>
> > Cc: David Airlie <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Chris Wilson <[email protected]>
> > Cc: Tvrtko Ursulin <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
>
> "drm/i915:" preferred as the subject prefix for increased specificity.

"drm/i915" it is!

> Let me know which tree you end up merging with.

I expect to be sending a pull request for inclusion into the -tip
tree in a day or three.

> Reviewed-by: Joonas Lahtinen <[email protected]>

Applied, thank you!

Thanx, Paul