4

The local_bh_disable-function changes per-cpu (in case of x86 and recent kernels) __preempt_count or current_thread_info()->preempt_count otherwise.

Anyway that gives us a grace period, so we can assume that it will be redundant to do rcu_read_lock() inside local_bh_disable(). Indeed: in earlier kernels we can see that local_bh_disable() is used for RCU and rcu_dereference() is called subsequently inside e.g. the dev_queue_xmit-function. Later local_bh_disable() was replaced with rcu_read_lock_bh(), which eventually became a little more complicated than just calling local_bh_disable(). Now it looks like this:

static inline void rcu_read_lock_bh(void)
{
   local_bh_disable();
   __acquire(RCU_BH);
   rcu_lock_acquire(&rcu_bh_lock_map);
   RCU_LOCKDEP_WARN(!rcu_is_watching(),"rcu_read_lock_bh() used illegally while idle");
}

Also there are enough articles describing the RCU APIs. Here we can see:

Do you need to treat NMI handlers, hardirq handlers, and code segments with preemption disabled (whether via preempt_disable(), local_irq_save(), local_bh_disable(), or some other mechanism) as if they were explicit RCU readers? If so, RCU-sched is the only choice that will work for you.

This tells us to use the RCU Sched API in such cases, so rcu_dereference_sched() should help. From this comprehensive table we can realise that rcu_dereference() should be used only inside rcu_read_lock/rcu_read_unlock-markers.

However, it is not sufficiently clear. Can I use (in case of modern kernels) rcu_dereference() inside local_bh_disable/local_bh_enable-markers without misgiving of anything going wrong?

P.S. In my case, I can't change the code calling local_bh_disable to call e.g. rcu_read_lock_bh, so my code is runnung with bh already disabled. Also the usual RCU API is used. Thus, it is fraught with rcu_read_lock nested in local_bh_disable.

red0ct
  • 4,840
  • 3
  • 17
  • 44

1 Answers1

3

You are not supposed to mix-and-match APIs. If you need to use the RCU-bh API, you need to use rcu_dereference_bh.

You can see that if you called rcu_dereference_check after rcu_read_lock_bh, it would correctly surmise it is not called in a RCU read-side critical section; contrast the call to lock_is_held(&rcu_lock_map) with rcu_lock_acquire(&rcu_bh_lock_map); in your snippet above.

The kernel documentation for RCU here (search for the section on "rcu_dereference()") gives an explicit example of correct usage; rcu_dereference* can only be called correctly after the corresponding rcu_read_lock* function has completed.

Michael Foukarakis
  • 39,737
  • 6
  • 87
  • 123
  • I've seen this documentation, but the first thing confused me was the usage in early kernels. And even more confused that the result of `rcu_read_lock*held` is used only for `RCU_LOCKDEP_WARN` inside `__rcu_dereference_check`... Does this mean that everything will work fine? – red0ct Jul 09 '18 at 12:23
  • I should use the standard RCU API, so only the `rcu_dereference` is welcome. Also I obviously shouldn't go against the latest documentation. So if I'm already inside `local_bh_disable` I just have to frame `rcu_dereference` with `rcu_read_lock`/`rcu_read_unlock`-markers and such *"preemption disabling"*-nesting will not be shameful? – red0ct Jul 09 '18 at 12:39
  • If you need to use RCU with preemption disabled (i.e. inside `local_bh_disable`) you can only use RCU-bh, there is no other alternative which will work correctly. – Michael Foukarakis Jul 09 '18 at 12:49
  • So you mean that the following sequence is incorrect? `local_bh_disable()`->`rcu_read_lock()`->`rcu_dereference()`->`rcu_read_unlock()`->`local_bh_enable()` – red0ct Jul 09 '18 at 12:55
  • Based on [this comment](https://elixir.bootlin.com/linux/v4.17.5/source/include/linux/rcupdate.h#L698), I can assume that the above sequence is still correct, isn't it? – red0ct Jul 09 '18 at 13:10
  • 1
    Perhaps it happened to work correctly in a certain kernel version. Generally - and currently - I can't make that statement; see for instance the defs of ` rcu_read_lock*_held`. It seems to me the comment is out of date. – Michael Foukarakis Jul 09 '18 at 13:36
  • 1
    Thank you for clarifying. Summing up, we can say that doing `local_bh_disable()`->`rcu_dereference()`->`local_bh_enable()` is incorrect usage nowdays. And [this above sequence](https://stackoverflow.com/questions/51236711/is-it-safe-to-use-rcu-dereference-inside-local-bh-disable-local-bh-enable/51244024?noredirect=1#comment89470747_51244024) is presumably correct based on comments in kernel source, but clarification is needed. – red0ct Jul 09 '18 at 13:55