On 8/27/20 8:10 AM, Jakub Kicinski wrote:
> On Thu, 27 Aug 2020 00:25:31 -0700 Eric Dumazet wrote:
>> On 8/26/20 12:40 PM, Jakub Kicinski wrote:
>>> To ensure memory ordering is correct we need to use RCU accessors.
>>
>>> + set_bit(NAPI_STATE_NPSVC, &napi->state);
>>> + list_add_rcu(&napi->dev_list, &dev->napi_list);
>>
>>>
>>> - list_for_each_entry(napi, &dev->napi_list, dev_list) {
>>> + list_for_each_entry_rcu(napi, &dev->napi_list, dev_list) {
>>> if (cmpxchg(&napi->poll_owner, -1, cpu) == -1) {
>>> poll_one_napi(napi);
>>> smp_store_release(&napi->poll_owner, -1);
>>>
>>
>> You added rcu in this patch (without anything in the changelog).
>
> I mentioned I need it for the barriers, in particular I wanted the
> store release barrier in list_add. Not extremely clean :(
Hmmm, we also have smp_mb__after_atomic()
>
>> netpoll_poll_dev() uses rcu_dereference_bh(), suggesting you might
>> need list_for_each_entry_rcu_bh()
>
> I thought the RCU flavors are mostly meaningless at this point,
> list_for_each_entry_rcu() checks rcu_read_lock_any_held(). I can add
> the definition of list_for_each_entry_rcu_bh() (since it doesn't exist)
> or go back to non-RCU iteration (since the use is just documentation,
> the code is identical). Or fix it some other way?
>
Oh, I really thought list_for_each_entry_rcu() was only checking standard rcu.
I might have been confused because we do have hlist_for_each_entry_rcu_bh()
helper.
Anyway, when looking at the patch I was not at ease because we do not have
proper
rcu grace period when a napi is removed from dev->napi_list. A driver might
free the napi struct right after calling netif_napi_del()
