On 08/12/2018 10:45 AM, Tariq Toukan wrote: > > > 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 ?
Please rebase against bpf-next and we route it to stable, thanks!