Messages in this thread | | | Date | Fri, 24 Mar 2023 23:09:08 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 1/4] rcu/nocb: Protect lazy shrinker against concurrent (de-)offloading |
| |
Le Wed, Mar 22, 2023 at 04:18:24PM -0700, Paul E. McKenney a écrit : > > @@ -1336,13 +1336,25 @@ lazy_rcu_shrink_scan(struct shrinker *shrink, struct shrink_control *sc) > > unsigned long flags; > > unsigned long count = 0; > > > > + /* > > + * Protect against concurrent (de-)offloading. Otherwise nocb locking > > + * may be ignored or imbalanced. > > + */ > > + mutex_lock(&rcu_state.barrier_mutex); > > I was worried about this possibly leading to out-of-memory deadlock, > but if I recall correctly, the (de-)offloading process never allocates > memory, so this should be OK?
Good point. It _should_ be fine but like you, Joel and Hillf pointed out it's asking for trouble.
We could try Joel's idea to use mutex_trylock() as a best effort, which should be fine as it's mostly uncontended.
The alternative is to force nocb locking and check the offloading state right after. So instead of:
rcu_nocb_lock_irqsave(rdp, flags); //flush stuff rcu_nocb_unlock_irqrestore(rdp, flags);
Have:
raw_spin_lock_irqsave(rdp->nocb_lock, flags); if (!rcu_rdp_is_offloaded(rdp)) raw_spin_unlock_irqrestore(rdp->nocb_lock, flags); continue; } //flush stuff rcu_nocb_unlock_irqrestore(rdp, flags);
But it's not pretty and also disqualifies the last two patches as rcu_nocb_mask can't be iterated safely anymore.
What do you think?
> > /* Snapshot count of all CPUs */ > > for_each_possible_cpu(cpu) { > > struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu); > > - int _count = READ_ONCE(rdp->lazy_len); > > + int _count; > > + > > + if (!rcu_rdp_is_offloaded(rdp)) > > + continue; > > If the CPU is offloaded, isn't ->lazy_len guaranteed to be zero? > > Or can it contain garbage after a de-offloading operation?
If it's deoffloaded, ->lazy_len is indeed (supposed to be) guaranteed to be zero. Bypass is flushed and disabled atomically early on de-offloading and the flush resets ->lazy_len.
Thanks.
| |