* [PATCH net] net/page_pool: Fix inconsistent lock state warning
@ 2018-07-17 15:10 Tariq Toukan
2018-07-17 15:10 ` [PATCH net] net/xdp: Fix suspicious RCU usage warning Tariq Toukan
2018-07-20 6:23 ` [PATCH net] net/page_pool: Fix inconsistent lock state warning David Miller
0 siblings, 2 replies; 9+ messages in thread
From: Tariq Toukan @ 2018-07-17 15:10 UTC (permalink / raw
To: David S. Miller
Cc: netdev, Eran Ben Elisha, Tariq Toukan, Jesper Dangaard Brouer
Fix the warning below by calling the ptr_ring_consume_bh,
which uses spin_[un]lock_bh.
[ 179.064300] ================================
[ 179.069073] WARNING: inconsistent lock state
[ 179.073846] 4.18.0-rc2+ #18 Not tainted
[ 179.078133] --------------------------------
[ 179.082907] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
[ 179.089637] swapper/21/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[ 179.095478] 00000000963d1995 (&(&r->consumer_lock)->rlock){+.?.}, at:
__page_pool_empty_ring+0x61/0x100
[ 179.105988] {SOFTIRQ-ON-W} state was registered at:
[ 179.111443] _raw_spin_lock+0x35/0x50
[ 179.115634] __page_pool_empty_ring+0x61/0x100
[ 179.120699] page_pool_destroy+0x32/0x50
[ 179.125204] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
[ 179.130471] mlx5e_close_channel+0x20/0x120 [mlx5_core]
[ 179.136418] mlx5e_close_channels+0x26/0x40 [mlx5_core]
[ 179.142364] mlx5e_close_locked+0x44/0x50 [mlx5_core]
[ 179.148509] mlx5e_close+0x42/0x60 [mlx5_core]
[ 179.153936] __dev_close_many+0xb1/0x120
[ 179.158749] dev_close_many+0xa2/0x170
[ 179.163364] rollback_registered_many+0x148/0x460
[ 179.169047] rollback_registered+0x56/0x90
[ 179.174043] unregister_netdevice_queue+0x7e/0x100
[ 179.179816] unregister_netdev+0x18/0x20
[ 179.184623] mlx5e_remove+0x2a/0x50 [mlx5_core]
[ 179.190107] mlx5_remove_device+0xe5/0x110 [mlx5_core]
[ 179.196274] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
[ 179.203028] cleanup+0x5/0xbfc [mlx5_core]
[ 179.208031] __x64_sys_delete_module+0x16b/0x240
[ 179.213640] do_syscall_64+0x5a/0x210
[ 179.218151] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 179.224218] irq event stamp: 334398
[ 179.228438] hardirqs last enabled at (334398): [<ffffffffa511d8b7>]
rcu_process_callbacks+0x1c7/0x790
[ 179.239178] hardirqs last disabled at (334397): [<ffffffffa511d872>]
rcu_process_callbacks+0x182/0x790
[ 179.249931] softirqs last enabled at (334386): [<ffffffffa509732e>] irq_enter+0x5e/0x70
[ 179.259306] softirqs last disabled at (334387): [<ffffffffa509741c>] irq_exit+0xdc/0xf0
[ 179.268584]
[ 179.268584] other info that might help us debug this:
[ 179.276572] Possible unsafe locking scenario:
[ 179.276572]
[ 179.283877] CPU0
[ 179.286954] ----
[ 179.290033] lock(&(&r->consumer_lock)->rlock);
[ 179.295546] <Interrupt>
[ 179.298830] lock(&(&r->consumer_lock)->rlock);
[ 179.304550]
[ 179.304550] *** DEADLOCK ***
Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
---
net/core/page_pool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 68bf07206744..43a932cb609b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -269,7 +269,7 @@ static void __page_pool_empty_ring(struct page_pool *pool)
struct page *page;
/* Empty recycle ring */
- while ((page = ptr_ring_consume(&pool->ring))) {
+ while ((page = ptr_ring_consume_bh(&pool->ring))) {
/* Verify the refcnt invariant of cached pages */
if (!(page_ref_count(page) == 1))
pr_crit("%s() page_pool refcnt %d violation\n",
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-17 15:10 [PATCH net] net/page_pool: Fix inconsistent lock state warning Tariq Toukan
@ 2018-07-17 15:10 ` Tariq Toukan
2018-07-17 16:47 ` Alexei Starovoitov
2018-07-20 6:23 ` [PATCH net] net/page_pool: Fix inconsistent lock state warning David Miller
1 sibling, 1 reply; 9+ messages in thread
From: Tariq Toukan @ 2018-07-17 15:10 UTC (permalink / raw
To: David S. Miller
Cc: netdev, Eran Ben Elisha, Tariq Toukan, Jesper Dangaard Brouer
Fix the warning below by calling rhashtable_lookup under
RCU read lock.
[ 342.450870] WARNING: suspicious RCU usage
[ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
[ 342.462210] -----------------------------
[ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
[ 342.476568]
[ 342.476568] other info that might help us debug this:
[ 342.476568]
[ 342.486978]
[ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
[ 342.495211] 4 locks held by modprobe/3934:
[ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
mlx5_unregister_interface+0x18/0x90 [mlx5_core]
[ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
[ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
[mlx5_core]
[ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
[ 342.541206]
[ 342.541206] stack backtrace:
[ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
[ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
[ 342.565606] Call Trace:
[ 342.568861] dump_stack+0x78/0xb3
[ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
[ 342.578285] ? __call_rcu+0x220/0x300
[ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
[ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
[ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
[ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
[ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
[ 342.613005] __dev_close_many+0xb1/0x120
[ 342.617911] dev_close_many+0xa2/0x170
[ 342.622622] rollback_registered_many+0x148/0x460
[ 342.628401] ? __lock_acquire+0x48d/0x11b0
[ 342.633498] ? unregister_netdev+0xe/0x20
[ 342.638495] rollback_registered+0x56/0x90
[ 342.643588] unregister_netdevice_queue+0x7e/0x100
[ 342.649461] unregister_netdev+0x18/0x20
[ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
[ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
[ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
[ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
[ 342.678094] __x64_sys_delete_module+0x16b/0x240
[ 342.683725] ? do_syscall_64+0x1c/0x210
[ 342.688476] do_syscall_64+0x5a/0x210
[ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
---
net/core/xdp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 9d1f22072d5d..c20fefbfb76c 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
mutex_lock(&mem_id_lock);
+ rcu_read_lock();
xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
+ rcu_read_unlock();
if (!xa) {
mutex_unlock(&mem_id_lock);
return;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-17 15:10 ` [PATCH net] net/xdp: Fix suspicious RCU usage warning Tariq Toukan
@ 2018-07-17 16:47 ` Alexei Starovoitov
2018-07-17 19:27 ` Daniel Borkmann
0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2018-07-17 16:47 UTC (permalink / raw
To: Tariq Toukan
Cc: David S. Miller, netdev, Eran Ben Elisha, Jesper Dangaard Brouer
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.
>
> [ 342.450870] WARNING: suspicious RCU usage
> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
> [ 342.462210] -----------------------------
> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
> [ 342.476568]
> [ 342.476568] other info that might help us debug this:
> [ 342.476568]
> [ 342.486978]
> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
> [ 342.495211] 4 locks held by modprobe/3934:
> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
> [mlx5_core]
> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
> [ 342.541206]
> [ 342.541206] stack backtrace:
> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
> [ 342.565606] Call Trace:
> [ 342.568861] dump_stack+0x78/0xb3
> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
> [ 342.578285] ? __call_rcu+0x220/0x300
> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
> [ 342.613005] __dev_close_many+0xb1/0x120
> [ 342.617911] dev_close_many+0xa2/0x170
> [ 342.622622] rollback_registered_many+0x148/0x460
> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
> [ 342.633498] ? unregister_netdev+0xe/0x20
> [ 342.638495] rollback_registered+0x56/0x90
> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
> [ 342.649461] unregister_netdev+0x18/0x20
> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
> [ 342.683725] ? do_syscall_64+0x1c/0x210
> [ 342.688476] do_syscall_64+0x5a/0x210
> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> net/core/xdp.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9d1f22072d5d..c20fefbfb76c 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-17 16:47 ` Alexei Starovoitov
@ 2018-07-17 19:27 ` Daniel Borkmann
2018-07-18 14:13 ` Tariq Toukan
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2018-07-17 19:27 UTC (permalink / raw
To: Alexei Starovoitov, Tariq Toukan
Cc: David S. Miller, netdev, Eran Ben Elisha, Jesper Dangaard Brouer
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.
>>
>> [ 342.450870] WARNING: suspicious RCU usage
>> [ 342.455856] 4.18.0-rc2+ #17 Tainted: G O
>> [ 342.462210] -----------------------------
>> [ 342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
>> [ 342.476568]
>> [ 342.476568] other info that might help us debug this:
>> [ 342.476568]
>> [ 342.486978]
>> [ 342.486978] rcu_scheduler_active = 2, debug_locks = 1
>> [ 342.495211] 4 locks held by modprobe/3934:
>> [ 342.500265] #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
>> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
>> [ 342.511953] #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
>> [ 342.521109] #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
>> [mlx5_core]
>> [ 342.531642] #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
>> [ 342.541206]
>> [ 342.541206] stack backtrace:
>> [ 342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G O 4.18.0-rc2+ #17
>> [ 342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
>> [ 342.565606] Call Trace:
>> [ 342.568861] dump_stack+0x78/0xb3
>> [ 342.573086] xdp_rxq_info_unreg+0x3f5/0x6b0
>> [ 342.578285] ? __call_rcu+0x220/0x300
>> [ 342.582911] mlx5e_free_rq+0x38/0xc0 [mlx5_core]
>> [ 342.588602] mlx5e_close_channel+0x20/0x120 [mlx5_core]
>> [ 342.594976] mlx5e_close_channels+0x26/0x40 [mlx5_core]
>> [ 342.601345] mlx5e_close_locked+0x44/0x50 [mlx5_core]
>> [ 342.607519] mlx5e_close+0x42/0x60 [mlx5_core]
>> [ 342.613005] __dev_close_many+0xb1/0x120
>> [ 342.617911] dev_close_many+0xa2/0x170
>> [ 342.622622] rollback_registered_many+0x148/0x460
>> [ 342.628401] ? __lock_acquire+0x48d/0x11b0
>> [ 342.633498] ? unregister_netdev+0xe/0x20
>> [ 342.638495] rollback_registered+0x56/0x90
>> [ 342.643588] unregister_netdevice_queue+0x7e/0x100
>> [ 342.649461] unregister_netdev+0x18/0x20
>> [ 342.654362] mlx5e_remove+0x2a/0x50 [mlx5_core]
>> [ 342.659944] mlx5_remove_device+0xe5/0x110 [mlx5_core]
>> [ 342.666208] mlx5_unregister_interface+0x39/0x90 [mlx5_core]
>> [ 342.673038] cleanup+0x5/0xbfc [mlx5_core]
>> [ 342.678094] __x64_sys_delete_module+0x16b/0x240
>> [ 342.683725] ? do_syscall_64+0x1c/0x210
>> [ 342.688476] do_syscall_64+0x5a/0x210
>> [ 342.693025] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
>> ---
>> net/core/xdp.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 9d1f22072d5d..c20fefbfb76c 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>>
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-17 19:27 ` Daniel Borkmann
@ 2018-07-18 14:13 ` Tariq Toukan
2018-07-19 21:36 ` Alexei Starovoitov
0 siblings, 1 reply; 9+ messages in thread
From: Tariq Toukan @ 2018-07-18 14:13 UTC (permalink / raw
To: Daniel Borkmann, Alexei Starovoitov, Tariq Toukan
Cc: David S. Miller, netdev, Eran Ben Elisha, Jesper Dangaard Brouer
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-18 14:13 ` Tariq Toukan
@ 2018-07-19 21:36 ` Alexei Starovoitov
2018-08-12 8:45 ` Tariq Toukan
0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2018-07-19 21:36 UTC (permalink / raw
To: Tariq Toukan
Cc: Daniel Borkmann, David S. Miller, netdev, Eran Ben Elisha,
Jesper Dangaard Brouer
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/page_pool: Fix inconsistent lock state warning
2018-07-17 15:10 [PATCH net] net/page_pool: Fix inconsistent lock state warning Tariq Toukan
2018-07-17 15:10 ` [PATCH net] net/xdp: Fix suspicious RCU usage warning Tariq Toukan
@ 2018-07-20 6:23 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: David Miller @ 2018-07-20 6:23 UTC (permalink / raw
To: tariqt; +Cc: netdev, eranbe, brouer
From: Tariq Toukan <tariqt@mellanox.com>
Date: Tue, 17 Jul 2018 18:10:37 +0300
> Fix the warning below by calling the ptr_ring_consume_bh,
> which uses spin_[un]lock_bh.
...
> Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-07-19 21:36 ` Alexei Starovoitov
@ 2018-08-12 8:45 ` Tariq Toukan
2018-08-12 23:04 ` Daniel Borkmann
0 siblings, 1 reply; 9+ messages in thread
From: Tariq Toukan @ 2018-08-12 8:45 UTC (permalink / raw
To: Alexei Starovoitov, Tariq Toukan
Cc: Daniel Borkmann, David S. Miller, netdev, Eran Ben Elisha,
Jesper Dangaard Brouer
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 ?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
2018-08-12 8:45 ` Tariq Toukan
@ 2018-08-12 23:04 ` Daniel Borkmann
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Borkmann @ 2018-08-12 23:04 UTC (permalink / raw
To: Tariq Toukan, Alexei Starovoitov
Cc: David S. Miller, netdev, Eran Ben Elisha, Jesper Dangaard Brouer
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-08-13 1:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-07-17 15:10 [PATCH net] net/page_pool: Fix inconsistent lock state warning Tariq Toukan
2018-07-17 15:10 ` [PATCH net] net/xdp: Fix suspicious RCU usage warning Tariq Toukan
2018-07-17 16:47 ` Alexei Starovoitov
2018-07-17 19:27 ` Daniel Borkmann
2018-07-18 14:13 ` Tariq Toukan
2018-07-19 21:36 ` Alexei Starovoitov
2018-08-12 8:45 ` Tariq Toukan
2018-08-12 23:04 ` Daniel Borkmann
2018-07-20 6:23 ` [PATCH net] net/page_pool: Fix inconsistent lock state warning David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.