On 20/07/2018 12:36 AM, Alexei Starovoitov wrote:
On Wed, Jul 18, 2018 at 05:13:54PM +0300, Tariq Toukan wrote:
On 17/07/2018 10:27 PM, Daniel Borkmann wrote:
On 07/17/2018 06:47 PM, Alexei Starovoitov wrote:
On Tue, Jul 17, 2018 at 06:10:38PM +0300, Tariq Toukan wrote:
Fix the warning below by calling rhashtable_lookup under
RCU read lock.
...
mutex_lock(&mem_id_lock);
+ rcu_read_lock();
xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
+ rcu_read_unlock();
if (!xa) {
if it's an actual bug rcu_read_unlock seems to be misplaced.
It silences the warn, but rcu section looks wrong.
I think that whole piece in __xdp_rxq_info_unreg_mem_model() should be:
mutex_lock(&mem_id_lock);
xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
if (xa && rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params)
== 0)
call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
mutex_unlock(&mem_id_lock);
Technically the RCU read side plus rhashtable_lookup() is the same, but lets
use proper api. From the doc (https://lwn.net/Articles/751374/) object removal
is wrapped around the RCU read side additionally, but in our case we're behind
mem_id_lock for insertion/removal serialization.
Cheers,
Daniel
Just as Daniel stated, I think there's no actual bug here, but we still want
to silence the RCU warning.
Alexei, did you mean getting the if statement into the RCU lock critical
section?
If what Daniel proposes silences the warn, I'd rather do that.
Pattern like:
rcu_lock;
val = lookup();
rcu_unlock;
if (val)
will cause people to question the quality of the code and whether
authors of the code understand rcu.
There should be a way to silence the warn without adding
"wrong on the first glance" code.
I'm re-spinning this.
Can it still go to net, or better send it to bpf-next ?
