offlist, since it seems this conversation is upsetting you.
Interesting, vger seems to be CCed but it isn't appearing on the ML.
Perhaps just a vger delay :S
Not really upsetting. I'm just trying to share what I learned devising
more advanced pollers. The bits get really messy really quickly.
Especially that the proposed fix adds a bit for a poor bystander (busy
poll) while it's the threaded IRQ that is incorrectly not preserving
its ownership.
Additional 16 bytes here, possibly in a shared cache line, [1]
I prefer using a bit in hot n->state, we have plenty of them available.
Right, presumably the location of the new member could be optimized.
I typed this proposal up in a couple of minutes.
We worked hours with Alexander, Wei, I am sorry you think we did a poor job.
I really thought we instead solved the issue at hand.
May I suggest you defer your idea of redesigning the NAPI model for
net-next ?
Seems like you decided on this solution off list and now the fact that
there is a discussion on the list is upsetting you. May I suggest that
discussions should be conducted on list to avoid such situations?
offlist, since it seems this conversation is upsetting you.
Interesting, vger seems to be CCed but it isn't appearing on the ML.
Perhaps just a vger delay :S
Not really upsetting. I'm just trying to share what I learned devising
more advanced pollers. The bits get really messy really quickly.
Especially that the proposed fix adds a bit for a poor bystander (busy
poll) while it's the threaded IRQ that is incorrectly not preserving
its ownership.
quoted
Additional 16 bytes here, possibly in a shared cache line, [1]
I prefer using a bit in hot n->state, we have plenty of them available.
Right, presumably the location of the new member could be optimized.
I typed this proposal up in a couple of minutes.
quoted
We worked hours with Alexander, Wei, I am sorry you think we did a poor job.
I really thought we instead solved the issue at hand.
May I suggest you defer your idea of redesigning the NAPI model for
net-next ?
Seems like you decided on this solution off list and now the fact that
there is a discussion on the list is upsetting you. May I suggest that
discussions should be conducted on list to avoid such situations?
We were trying to not pollute the list (with about 40 different emails so far)
(Note this was not something I initiated, I only hit Reply all button)
OK, I will shut up, since you seem to take over this matter, and it is
1am here in France.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 00:08:27
On Thu, 25 Feb 2021 00:59:25 +0100 Eric Dumazet wrote:
On Thu, Feb 25, 2021 at 12:52 AM Jakub Kicinski [off-list ref] wrote:
quoted
Interesting, vger seems to be CCed but it isn't appearing on the ML.
Perhaps just a vger delay :S
Not really upsetting. I'm just trying to share what I learned devising
more advanced pollers. The bits get really messy really quickly.
Especially that the proposed fix adds a bit for a poor bystander (busy
poll) while it's the threaded IRQ that is incorrectly not preserving
its ownership.
quoted
Additional 16 bytes here, possibly in a shared cache line, [1]
I prefer using a bit in hot n->state, we have plenty of them available.
Right, presumably the location of the new member could be optimized.
I typed this proposal up in a couple of minutes.
quoted
We worked hours with Alexander, Wei, I am sorry you think we did a poor job.
I really thought we instead solved the issue at hand.
May I suggest you defer your idea of redesigning the NAPI model for
net-next ?
Seems like you decided on this solution off list and now the fact that
there is a discussion on the list is upsetting you. May I suggest that
discussions should be conducted on list to avoid such situations?
We were trying to not pollute the list (with about 40 different emails so far)
(Note this was not something I initiated, I only hit Reply all button)
OK, I will shut up, since you seem to take over this matter, and it is
1am here in France.
Are you okay with adding a SCHED_THREADED bit for threaded NAPI to be
set in addition to SCHED? At least that way the bit is associated with
it's user. IIUC since the extra clear_bit() in busy poll was okay so
should be a new set_bit()?
From: Alexander Duyck <alexanderduyck@fb.com> Date: 2021-02-25 00:12:46
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, February 24, 2021 4:07 PM
To: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>; David S . Miller
[off-list ref]; netdev [off-list ref]; Paolo Abeni
[off-list ref]; Hannes Frederic Sowa
[off-list ref]; Alexander Duyck
[off-list ref]; Martin Zaharinov [off-list ref]
Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy
poll
On Thu, 25 Feb 2021 00:59:25 +0100 Eric Dumazet wrote:
quoted
On Thu, Feb 25, 2021 at 12:52 AM Jakub Kicinski [off-list ref] wrote:
quoted
Interesting, vger seems to be CCed but it isn't appearing on the ML.
Perhaps just a vger delay :S
Not really upsetting. I'm just trying to share what I learned
devising more advanced pollers. The bits get really messy really quickly.
Especially that the proposed fix adds a bit for a poor bystander
(busy
poll) while it's the threaded IRQ that is incorrectly not preserving
its ownership.
quoted
Additional 16 bytes here, possibly in a shared cache line, [1] I
prefer using a bit in hot n->state, we have plenty of them available.
Right, presumably the location of the new member could be optimized.
I typed this proposal up in a couple of minutes.
quoted
We worked hours with Alexander, Wei, I am sorry you think we did a
poor job.
quoted
quoted
quoted
I really thought we instead solved the issue at hand.
May I suggest you defer your idea of redesigning the NAPI model
for net-next ?
Seems like you decided on this solution off list and now the fact
that there is a discussion on the list is upsetting you. May I
suggest that discussions should be conducted on list to avoid such
situations?
quoted
We were trying to not pollute the list (with about 40 different emails
so far)
(Note this was not something I initiated, I only hit Reply all button)
OK, I will shut up, since you seem to take over this matter, and it is
1am here in France.
Are you okay with adding a SCHED_THREADED bit for threaded NAPI to be
set in addition to SCHED? At least that way the bit is associated with it's user.
IIUC since the extra clear_bit() in busy poll was okay so should be a new
set_bit()?
The problem with adding a bit for SCHED_THREADED is that you would have to heavily modify napi_schedule_prep so that it would add the bit. That is the reason for going with adding the bit to the busy poll logic because it added no additional overhead. Adding another atomic bit setting operation or heavily modifying the existing one would add considerable overhead as it is either adding a complicated conditional check to all NAPI calls, or adding an atomic operation to the path for the threaded NAPI.
On Wed, Feb 24, 2021 at 4:11 PM Alexander Duyck [off-list ref] wrote:
quoted
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, February 24, 2021 4:07 PM
To: Eric Dumazet <edumazet@google.com>
Cc: Wei Wang <redacted>; David S . Miller
[off-list ref]; netdev [off-list ref]; Paolo Abeni
[off-list ref]; Hannes Frederic Sowa
[off-list ref]; Alexander Duyck
[off-list ref]; Martin Zaharinov [off-list ref]
Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy
poll
On Thu, 25 Feb 2021 00:59:25 +0100 Eric Dumazet wrote:
quoted
On Thu, Feb 25, 2021 at 12:52 AM Jakub Kicinski [off-list ref] wrote:
quoted
Interesting, vger seems to be CCed but it isn't appearing on the ML.
Perhaps just a vger delay :S
Not really upsetting. I'm just trying to share what I learned
devising more advanced pollers. The bits get really messy really quickly.
Especially that the proposed fix adds a bit for a poor bystander
(busy
poll) while it's the threaded IRQ that is incorrectly not preserving
its ownership.
quoted
Additional 16 bytes here, possibly in a shared cache line, [1] I
prefer using a bit in hot n->state, we have plenty of them available.
Right, presumably the location of the new member could be optimized.
I typed this proposal up in a couple of minutes.
quoted
We worked hours with Alexander, Wei, I am sorry you think we did a
poor job.
quoted
quoted
quoted
I really thought we instead solved the issue at hand.
May I suggest you defer your idea of redesigning the NAPI model
for net-next ?
Seems like you decided on this solution off list and now the fact
that there is a discussion on the list is upsetting you. May I
suggest that discussions should be conducted on list to avoid such
situations?
quoted
We were trying to not pollute the list (with about 40 different emails
so far)
(Note this was not something I initiated, I only hit Reply all button)
OK, I will shut up, since you seem to take over this matter, and it is
1am here in France.
Are you okay with adding a SCHED_THREADED bit for threaded NAPI to be
set in addition to SCHED? At least that way the bit is associated with it's user.
IIUC since the extra clear_bit() in busy poll was okay so should be a new
set_bit()?
The problem with adding a bit for SCHED_THREADED is that you would have to heavily modify napi_schedule_prep so that it would add the bit. That is the reason for going with adding the bit to the busy poll logic because it added no additional overhead. Adding another atomic bit setting operation or heavily modifying the existing one would add considerable overhead as it is either adding a complicated conditional check to all NAPI calls, or adding an atomic operation to the path for the threaded NAPI.
Please help hold on to the patch for now. I think Martin is still
seeing issues on his setup even with this patch applied. I have not
yet figured out why. But I think we should not merge this patch until
the issue is cleared. Will update this thread with progress.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 00:21:46
On Thu, 25 Feb 2021 00:11:34 +0000 Alexander Duyck wrote:
quoted
quoted
We were trying to not pollute the list (with about 40 different emails
so far)
(Note this was not something I initiated, I only hit Reply all button)
OK, I will shut up, since you seem to take over this matter, and it is
1am here in France.
Are you okay with adding a SCHED_THREADED bit for threaded NAPI to be
set in addition to SCHED? At least that way the bit is associated with it's user.
IIUC since the extra clear_bit() in busy poll was okay so should be a new
set_bit()?
The problem with adding a bit for SCHED_THREADED is that you would
have to heavily modify napi_schedule_prep so that it would add the
bit. That is the reason for going with adding the bit to the busy
poll logic because it added no additional overhead. Adding another
atomic bit setting operation or heavily modifying the existing one
would add considerable overhead as it is either adding a complicated
conditional check to all NAPI calls, or adding an atomic operation to
the path for the threaded NAPI.
I wasn't thinking of modifying the main schedule logic, just the
threaded parts:
@@ -360,6 +360,7 @@ enum {NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */NAPI_STATE_PREFER_BUSY_POLL,/* prefer busy-polling over softirq processing*/NAPI_STATE_THREADED,/* The poll is performed inside its own thread*/+NAPI_STATE_SCHED_THREAD,/* Thread owns the NAPI and will poll */};enum{
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 00:33:46
On Wed, 24 Feb 2021 16:16:58 -0800 Wei Wang wrote:
On Wed, Feb 24, 2021 at 4:11 PM Alexander Duyck [off-list ref] wrote:
quoted
The problem with adding a bit for SCHED_THREADED is that you would
have to heavily modify napi_schedule_prep so that it would add the
bit. That is the reason for going with adding the bit to the busy
poll logic because it added no additional overhead. Adding another
atomic bit setting operation or heavily modifying the existing one
would add considerable overhead as it is either adding a
complicated conditional check to all NAPI calls, or adding an
atomic operation to the path for the threaded NAPI.
Please help hold on to the patch for now. I think Martin is still
seeing issues on his setup even with this patch applied. I have not
yet figured out why. But I think we should not merge this patch until
the issue is cleared. Will update this thread with progress.
If I'm looking right __busy_poll_stop() is only called if the last
napi poll used to re-enable IRQs consumed full budget. You need to
clear your new bit in busy_poll_stop(), not in __busy_poll_stop().
That will fix the case when hand off back to the normal poller (sirq,
or thread) happens without going thru __napi_schedule().
On Wed, Feb 24, 2021 at 4:33 PM Jakub Kicinski [off-list ref] wrote:
On Wed, 24 Feb 2021 16:16:58 -0800 Wei Wang wrote:
quoted
On Wed, Feb 24, 2021 at 4:11 PM Alexander Duyck [off-list ref] wrote:
quoted
The problem with adding a bit for SCHED_THREADED is that you would
have to heavily modify napi_schedule_prep so that it would add the
bit. That is the reason for going with adding the bit to the busy
poll logic because it added no additional overhead. Adding another
atomic bit setting operation or heavily modifying the existing one
would add considerable overhead as it is either adding a
complicated conditional check to all NAPI calls, or adding an
atomic operation to the path for the threaded NAPI.
Please help hold on to the patch for now. I think Martin is still
seeing issues on his setup even with this patch applied. I have not
yet figured out why. But I think we should not merge this patch until
the issue is cleared. Will update this thread with progress.
If I'm looking right __busy_poll_stop() is only called if the last
napi poll used to re-enable IRQs consumed full budget. You need to
clear your new bit in busy_poll_stop(), not in __busy_poll_stop().
That will fix the case when hand off back to the normal poller (sirq,
or thread) happens without going thru __napi_schedule().
If the budget is not fully consumed, napi_complete_done() should have
been called by the driver which will clear SCHED_BUSY_POLL bit.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 00:50:34
On Wed, 24 Feb 2021 16:44:55 -0800 Wei Wang wrote:
On Wed, Feb 24, 2021 at 4:33 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Wed, 24 Feb 2021 16:16:58 -0800 Wei Wang wrote:
quoted
On Wed, Feb 24, 2021 at 4:11 PM Alexander Duyck [off-list ref] wrote:
[...]
quoted
quoted
Please help hold on to the patch for now. I think Martin is still
seeing issues on his setup even with this patch applied. I have not
yet figured out why. But I think we should not merge this patch until
the issue is cleared. Will update this thread with progress.
If I'm looking right __busy_poll_stop() is only called if the last
napi poll used to re-enable IRQs consumed full budget. You need to
clear your new bit in busy_poll_stop(), not in __busy_poll_stop().
That will fix the case when hand off back to the normal poller (sirq,
or thread) happens without going thru __napi_schedule().
If the budget is not fully consumed, napi_complete_done() should have
been called by the driver which will clear SCHED_BUSY_POLL bit.
I really have a hard time reproducing the warning Martin was seeing in
his setup. The difference between my setup and his is that mine uses
mlx4 driver, while Martin is using ixgbe driver.
To keep everyone up to date with Martin's previous email, with this
patch applied to 5.11.1, the following warning is triggered when
enabling threaded mode without enabling busy poll:
echo 1 > /sys/class/net/eth0/threaded
echo 1 > /sys/class/net/eth1/threaded
echo 1 > /sys/class/net/eth2/threaded
echo 1 > /sys/class/net/eth3/threaded
Warning message:
[ 92.883326] WARNING: CPU: 10 PID: 37100 at net/core/dev.c:6993
napi_threaded_poll+0x144/0x150
[ 92.883333] Modules linked in: iptable_filter xt_TCPMSS
iptable_mangle xt_addrtype xt_nat iptable_nat ip_tables pppoe pppox
ppp_generic slhc team_mode_loadbalance team netconsole coretemp ixgbe
mdio mdio_devres libphy
[ 92.884616] ip_tables pppoe pppox ppp_generic slhc
team_mode_loadbalance team netconsole coretemp ixgbe mdio mdio_devres
libphy nf_nat_sip nf_conntrack_sip nf_nat_pptp nf_conntrack_pptp
[ 92.886001] nf_nat_sip
[ 92.887262] nf_nat_tftp
[ 92.891169] nf_conntrack_sip
[ 92.894696] nf_conntrack_tftp nf_nat_ftp nf_conntrack_ftp nf_nat
nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 acpi_ipmi ipmi_si
ipmi_devintf ipmi_msghandler rtc_cmos
[ 92.894705] CPU: 18 PID: 37132 Comm: napi/eth3-703 Tainted: G
O 5.11.1 #1
[ 92.895133] nf_nat_pptp
[ 92.895570] Hardware name: Supermicro Super Server/X10DRi-LN4+,
BIOS 3.2 11/19/2019
[ 92.895572] RIP: 0010:napi_threaded_poll+0x144/0x150
[ 92.895576] Code: 83 e8 f0 2b 9f ff 80 7c 24 07 00 0f 84 e9 fe ff
ff e8 40 75 1f 00 e9 77 ff ff ff 48 8d 74 24 07 48 89 df e8 2e fd ff
ff eb cb <0f> 0b e9 53 ff ff ff 0f 1f 44 00 00 41 57 41 56 41 55 41 54
55 53
[ 92.896097] nf_conntrack_pptp
[ 92.898490] RSP: 0018:ffffa3af62857ee0 EFLAGS: 00010287
[ 92.898493] RAX: ffffa3af434fcf50 RBX: ffff94e5da281050 RCX: 0000000000000000
[ 92.898494] RDX: 0000000000000001 RSI: 0000000000000246 RDI: ffff94e5da281050
[ 92.898495] RBP: ffff94e60f463b00 R08: ffff94e9afa21758 R09: ffff94e9afa21758
[ 92.898496] R10: 0000000000000000 R11: 0000000000000000 R12: ffff94e5db0bf800
[ 92.898497] R13: ffffa3af4521fd10 R14: ffff94e5da281050 R15: ffff94e60f463b00
[ 92.898499] FS: 0000000000000000(0000) GS:ffff94e9afa00000(0000)
knlGS:0000000000000000
[ 92.898501] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 92.898502] CR2: 00007f76000a1b60 CR3: 00000001db40a005 CR4: 00000000001706e0
[ 92.898503] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 92.898504] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 92.898506] Call Trace:
[ 92.898508] ? __kthread_parkme+0x43/0x60
[ 92.898514] ? __napi_poll+0x190/0x190
[ 92.898516] kthread+0xea/0x120
[ 92.898520] ? kthread_park+0x80/0x80
[ 92.898523] ret_from_fork+0x1f/0x30
[ 92.898527] ---[ end trace 51046c7b7172e5a2 ]---
This is the line in net/core/dev.c:6993
WARN_ON(!list_empty(&napi->poll_list));
in napi_threaded wait()
Martin, do you think the driver version you are using could be at fault here?
On Wed, Feb 24, 2021 at 4:49 PM Jakub Kicinski [off-list ref] wrote:
On Wed, 24 Feb 2021 16:44:55 -0800 Wei Wang wrote:
quoted
On Wed, Feb 24, 2021 at 4:33 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Wed, 24 Feb 2021 16:16:58 -0800 Wei Wang wrote:
quoted
On Wed, Feb 24, 2021 at 4:11 PM Alexander Duyck [off-list ref] wrote:
[...]
quoted
quoted
Please help hold on to the patch for now. I think Martin is still
seeing issues on his setup even with this patch applied. I have not
yet figured out why. But I think we should not merge this patch until
the issue is cleared. Will update this thread with progress.
If I'm looking right __busy_poll_stop() is only called if the last
napi poll used to re-enable IRQs consumed full budget. You need to
clear your new bit in busy_poll_stop(), not in __busy_poll_stop().
That will fix the case when hand off back to the normal poller (sirq,
or thread) happens without going thru __napi_schedule().
If the budget is not fully consumed, napi_complete_done() should have
been called by the driver which will clear SCHED_BUSY_POLL bit.
From: Alexander Duyck <alexanderduyck@fb.com> Date: 2021-02-25 01:23:16
quoted hunk
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, February 24, 2021 4:21 PM
To: Alexander Duyck <alexanderduyck@fb.com>
Cc: Eric Dumazet <edumazet@google.com>; Wei Wang
[off-list ref]; David S . Miller [off-list ref]; netdev
[off-list ref]; Paolo Abeni [off-list ref]; Hannes
Frederic Sowa [off-list ref]; Martin Zaharinov
[off-list ref]
Subject: Re: [PATCH net] net: fix race between napi kthread mode and busy
poll
On Thu, 25 Feb 2021 00:11:34 +0000 Alexander Duyck wrote:
quoted
quoted
quoted
We were trying to not pollute the list (with about 40 different
emails so far)
(Note this was not something I initiated, I only hit Reply all
button)
OK, I will shut up, since you seem to take over this matter, and
it is 1am here in France.
Are you okay with adding a SCHED_THREADED bit for threaded NAPI to
be set in addition to SCHED? At least that way the bit is associated with it's
user.
quoted
quoted
IIUC since the extra clear_bit() in busy poll was okay so should be
a new set_bit()?
The problem with adding a bit for SCHED_THREADED is that you would
have to heavily modify napi_schedule_prep so that it would add the
bit. That is the reason for going with adding the bit to the busy poll
logic because it added no additional overhead. Adding another atomic
bit setting operation or heavily modifying the existing one would add
considerable overhead as it is either adding a complicated conditional
check to all NAPI calls, or adding an atomic operation to the path for
the threaded NAPI.
I wasn't thinking of modifying the main schedule logic, just the threaded
parts:
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index
@@ -360,6 +360,7 @@ enum {NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */NAPI_STATE_PREFER_BUSY_POLL,/* prefer busy-polling over softirq
processing*/
NAPI_STATE_THREADED, /* The poll is performed inside its own
thread*/
+ NAPI_STATE_SCHED_THREAD, /* Thread owns the NAPI and will poll
*/
};
enum {
diff --git a/net/core/dev.c b/net/core/dev.c index
int work_done)
WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));
new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
- NAPIF_STATE_PREFER_BUSY_POLL);
+ NAPIF_STATE_PREFER_BUSY_POLL |
+ NAPI_STATE_SCHED_THREAD);
/* If STATE_MISSED was set, leave STATE_SCHED set,
* because we will call napi->poll() one more time.
@@ -6971,7 +6973,9 @@ static int napi_thread_wait(struct napi_struct
*napi)
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop() && !napi_disable_pending(napi)) {
- if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
+ if (test_bit(NAPI_STATE_SCHED_THREAD, &napi->state)) {
+ WARN_ON(!test_bit(test_bit(NAPI_STATE_SCHED,
+ &napi->state)));
WARN_ON(!list_empty(&napi->poll_list));
__set_current_state(TASK_RUNNING);
return 0;
Yeah, that was the patch Wei had done earlier. Eric complained about the extra set_bit atomic operation in the threaded path. That is when I came up with the idea of just adding a bit to the busy poll logic so that the only extra cost in the threaded path was having to check 2 bits instead of 1.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 01:41:47
On Wed, 24 Feb 2021 17:06:13 -0800 Wei Wang wrote:
I really have a hard time reproducing the warning Martin was seeing in
his setup. The difference between my setup and his is that mine uses
mlx4 driver, while Martin is using ixgbe driver.
To keep everyone up to date with Martin's previous email, with this
patch applied to 5.11.1, the following warning is triggered when
enabling threaded mode without enabling busy poll:
echo 1 > /sys/class/net/eth0/threaded
echo 1 > /sys/class/net/eth1/threaded
echo 1 > /sys/class/net/eth2/threaded
echo 1 > /sys/class/net/eth3/threaded
Warning message:
[...]
This is the line in net/core/dev.c:6993
WARN_ON(!list_empty(&napi->poll_list));
in napi_threaded wait()
Martin, do you think the driver version you are using could be at fault here?
We do kthread_run() meaning the thread gets immediately woken up, even
when sirq is polling the NAPI and owns it. Right?
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 02:04:30
On Thu, 25 Feb 2021 01:22:08 +0000 Alexander Duyck wrote:
Yeah, that was the patch Wei had done earlier. Eric complained about the extra set_bit atomic operation in the threaded path. That is when I came up with the idea of just adding a bit to the busy poll logic so that the only extra cost in the threaded path was having to check 2 bits instead of 1.
Maybe we can set the bit only if the thread is running? When thread
comes out of schedule() it can be sure that it has an NAPI to service.
But when it enters napi_thread_wait() and before it hits schedule()
it must be careful to make sure the NAPI is still (or already in the
very first run after creation) owned by it.
On Wed, Feb 24, 2021 at 5:40 PM Jakub Kicinski [off-list ref] wrote:
On Wed, 24 Feb 2021 17:06:13 -0800 Wei Wang wrote:
quoted
I really have a hard time reproducing the warning Martin was seeing in
his setup. The difference between my setup and his is that mine uses
mlx4 driver, while Martin is using ixgbe driver.
To keep everyone up to date with Martin's previous email, with this
patch applied to 5.11.1, the following warning is triggered when
enabling threaded mode without enabling busy poll:
echo 1 > /sys/class/net/eth0/threaded
echo 1 > /sys/class/net/eth1/threaded
echo 1 > /sys/class/net/eth2/threaded
echo 1 > /sys/class/net/eth3/threaded
Warning message:
[...]
This is the line in net/core/dev.c:6993
WARN_ON(!list_empty(&napi->poll_list));
in napi_threaded wait()
Martin, do you think the driver version you are using could be at fault here?
We do kthread_run() meaning the thread gets immediately woken up, even
when sirq is polling the NAPI and owns it. Right?
Indeed. Good catch! Changing it to kthread_create() should solve the
issue I think. We were using kthread_run() because kthread_create()
messes up certain thread stats if I remember it correctly.
On Wed, Feb 24, 2021 at 6:03 PM Jakub Kicinski [off-list ref] wrote:
On Thu, 25 Feb 2021 01:22:08 +0000 Alexander Duyck wrote:
quoted
Yeah, that was the patch Wei had done earlier. Eric complained about the extra set_bit atomic operation in the threaded path. That is when I came up with the idea of just adding a bit to the busy poll logic so that the only extra cost in the threaded path was having to check 2 bits instead of 1.
Maybe we can set the bit only if the thread is running? When thread
comes out of schedule() it can be sure that it has an NAPI to service.
But when it enters napi_thread_wait() and before it hits schedule()
it must be careful to make sure the NAPI is still (or already in the
very first run after creation) owned by it.
Are you suggesting setting the SCHED_THREAD bit in napi_thread_wait()
somewhere instead of in ____napi_schedule() as you previously plotted?
What does it help? I think if we have to do an extra set_bit(), it
seems cleaner to set it in ____napi_schedule(). This would solve the
warning issue as well.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 08:22:04
On Wed, 24 Feb 2021 18:31:55 -0800 Wei Wang wrote:
On Wed, Feb 24, 2021 at 6:03 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 01:22:08 +0000 Alexander Duyck wrote:
quoted
Yeah, that was the patch Wei had done earlier. Eric complained about the extra set_bit atomic operation in the threaded path. That is when I came up with the idea of just adding a bit to the busy poll logic so that the only extra cost in the threaded path was having to check 2 bits instead of 1.
Maybe we can set the bit only if the thread is running? When thread
comes out of schedule() it can be sure that it has an NAPI to service.
But when it enters napi_thread_wait() and before it hits schedule()
it must be careful to make sure the NAPI is still (or already in the
very first run after creation) owned by it.
Are you suggesting setting the SCHED_THREAD bit in napi_thread_wait()
somewhere instead of in ____napi_schedule() as you previously plotted?
What does it help? I think if we have to do an extra set_bit(), it
seems cleaner to set it in ____napi_schedule(). This would solve the
warning issue as well.
@@ -360,6 +360,7 @@ enum {NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */NAPI_STATE_PREFER_BUSY_POLL,/* prefer busy-polling over softirq processing*/NAPI_STATE_THREADED,/* The poll is performed inside its own thread*/+NAPI_STATE_SCHED_THREAD,/* Thread owns the NAPI and will poll */};enum{
Extra set_bit() is only done if napi_schedule() comes early enough to
see the thread still running. When the thread is woken we continue to
assume ownership.
It's just an idea (but it may solve the first run and the disable case).
On Thu, Feb 25, 2021 at 12:21 AM Jakub Kicinski [off-list ref] wrote:
quoted hunk
On Wed, 24 Feb 2021 18:31:55 -0800 Wei Wang wrote:
quoted
On Wed, Feb 24, 2021 at 6:03 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 01:22:08 +0000 Alexander Duyck wrote:
quoted
Yeah, that was the patch Wei had done earlier. Eric complained about the extra set_bit atomic operation in the threaded path. That is when I came up with the idea of just adding a bit to the busy poll logic so that the only extra cost in the threaded path was having to check 2 bits instead of 1.
Maybe we can set the bit only if the thread is running? When thread
comes out of schedule() it can be sure that it has an NAPI to service.
But when it enters napi_thread_wait() and before it hits schedule()
it must be careful to make sure the NAPI is still (or already in the
very first run after creation) owned by it.
Are you suggesting setting the SCHED_THREAD bit in napi_thread_wait()
somewhere instead of in ____napi_schedule() as you previously plotted?
What does it help? I think if we have to do an extra set_bit(), it
seems cleaner to set it in ____napi_schedule(). This would solve the
warning issue as well.
@@ -360,6 +360,7 @@ enum {NAPI_STATE_IN_BUSY_POLL,/* sk_busy_loop() owns this NAPI */NAPI_STATE_PREFER_BUSY_POLL,/* prefer busy-polling over softirq processing*/NAPI_STATE_THREADED,/* The poll is performed inside its own thread*/+NAPI_STATE_SCHED_THREAD,/* Thread owns the NAPI and will poll */};enum{
Extra set_bit() is only done if napi_schedule() comes early enough to
see the thread still running. When the thread is woken we continue to
assume ownership.
It's just an idea (but it may solve the first run and the disable case).
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait(). woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-25 23:01:56
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
quoted
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.
quoted
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
quoted
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.
Thanks.
Wei
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-26 01:19:53
On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote:
On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
quoted
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.
Are you concerned about it not being in RUNNING state after it's
spawned but before it's first parked?
quoted
quoted
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
quoted
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.
To be clear extra set_bit() only if thread is running, which if IRQ
coalescing works should be rather rare.
On Thu, Feb 25, 2021 at 5:19 PM Jakub Kicinski [off-list ref] wrote:
On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote:
quoted
On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
quoted
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.
Are you concerned about it not being in RUNNING state after it's
spawned but before it's first parked?
I think it's fine. As long as ___napi_schedule() calls
wake_up_process() to wake up the thread when in threaded mode, I think
that serves the purpose.
quoted
quoted
quoted
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
quoted
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.
To be clear extra set_bit() only if thread is running, which if IRQ
coalescing works should be rather rare.
From: Alexander Duyck <hidden> Date: 2021-02-26 03:52:59
On Thu, Feb 25, 2021 at 5:20 PM Jakub Kicinski [off-list ref] wrote:
On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote:
quoted
On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
quoted
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.
Are you concerned about it not being in RUNNING state after it's
spawned but before it's first parked?
quoted
quoted
quoted
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
quoted
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.
To be clear extra set_bit() only if thread is running, which if IRQ
coalescing works should be rather rare.
I was good with either approach. My preference would be to probably
use kthread_create regardless as it doesn't make much sense to have
the thread running until we really need it anyway.
from poll_list */
NAPI_STATE_LISTED, /* NAPI added to system lists */
NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no
busy polling */
- NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */
+ NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() grabs SHED
bit and could busy poll */
NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over
softirq processing*/
NAPI_STATE_THREADED, /* The poll is performed
inside its own thread*/
+ NAPI_STATE_SCHED_BUSY_POLL, /* Napi is currently scheduled
in busy poll mode */
};
enum {
@@ -1501,15 +1501,14 @@ static int napi_kthread_create(struct napi_struct *n){interr=0;-/* Create and wake up the kthread once to put it in-*TASK_INTERRUPTIBLEmodetoavoidtheblockedtask-*warningandworkwithloadavg.+/* Avoid using kthread_run() here to prevent race+*betweensoftirqandkthreadpolling.*/-n->thread=kthread_run(napi_threaded_poll,n,"napi/%s-%d",-n->dev->name,n->napi_id);+n->thread=kthread_create(napi_threaded_poll,n,"napi/%s-%d",+n->dev->name,n->napi_id);if(IS_ERR(n->thread)){err=PTR_ERR(n->thread);-pr_err("kthread_run failed with err %d\n",err);+pr_err("kthread_create failed with err %d\n",err);n->thread=NULL;}
int work_done)
WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));
new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
+ NAPIF_STATE_SCHED_BUSY_POLL |
NAPIF_STATE_PREFER_BUSY_POLL);
/* If STATE_MISSED was set, leave STATE_SCHED set,
@@ -6971,7 +6973,10 @@ static int napi_thread_wait(struct napi_struct *napi) set_current_state(TASK_INTERRUPTIBLE); while (!kthread_should_stop() && !napi_disable_pending(napi)) {- if (test_bit(NAPI_STATE_SCHED, &napi->state)) {+ unsigned long val = READ_ONCE(napi->state);++ if (val & NAPIF_STATE_SCHED &&+ !(val & NAPIF_STATE_SCHED_BUSY_POLL)) { WARN_ON(!list_empty(&napi->poll_list)); __set_current_state(TASK_RUNNING); return 0;
On Thu, Feb 25, 2021 at 7:52 PM Alexander Duyck
[off-list ref] wrote:
On Thu, Feb 25, 2021 at 5:20 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote:
quoted
On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:
quoted
On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:
quoted
Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().
But how is the task not in running state outside of napi_thread_wait()?
My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().
I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.
Are you concerned about it not being in RUNNING state after it's
spawned but before it's first parked?
quoted
quoted
quoted
woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.
Probably just me not understanding the scheduler :)
quoted
I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().
Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.
Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.
To be clear extra set_bit() only if thread is running, which if IRQ
coalescing works should be rather rare.
I was good with either approach. My preference would be to probably
use kthread_create regardless as it doesn't make much sense to have
the thread running until we really need it anyway.
from poll_list */
NAPI_STATE_LISTED, /* NAPI added to system lists */
NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no
busy polling */
- NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */
+ NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() grabs SHED
nit: SHED -> SCHED
bit and could busy poll */
NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over
softirq processing*/
NAPI_STATE_THREADED, /* The poll is performed
inside its own thread*/
+ NAPI_STATE_SCHED_BUSY_POLL, /* Napi is currently scheduled
in busy poll mode */
@@ -1501,15 +1501,14 @@ static int napi_kthread_create(struct napi_struct *n){interr=0;-/* Create and wake up the kthread once to put it in-*TASK_INTERRUPTIBLEmodetoavoidtheblockedtask-*warningandworkwithloadavg.+/* Avoid using kthread_run() here to prevent race+*betweensoftirqandkthreadpolling.*/-n->thread=kthread_run(napi_threaded_poll,n,"napi/%s-%d",-n->dev->name,n->napi_id);+n->thread=kthread_create(napi_threaded_poll,n,"napi/%s-%d",+n->dev->name,n->napi_id);
I'm not sure this takes care of rapid:
dev_set_threaded(0)
# NAPI gets sent to sirq
dev_set_threaded(1)
since subsequent set_threaded(1) doesn't spawn the thread "afresh".
quoted hunk
if (IS_ERR(n->thread)) {
err = PTR_ERR(n->thread);
- pr_err("kthread_run failed with err %d\n", err);
+ pr_err("kthread_create failed with err %d\n", err);
n->thread = NULL;
}
int work_done)
WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));
new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
+ NAPIF_STATE_SCHED_BUSY_POLL |
NAPIF_STATE_PREFER_BUSY_POLL);
/* If STATE_MISSED was set, leave STATE_SCHED set,
from poll_list */
NAPI_STATE_LISTED, /* NAPI added to system lists */
NAPI_STATE_NO_BUSY_POLL, /* Do not add in napi_hash, no
busy polling */
- NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() owns this NAPI */
+ NAPI_STATE_IN_BUSY_POLL, /* sk_busy_loop() grabs SHED
nit: SHED -> SCHED
Ack.
quoted
bit and could busy poll */
NAPI_STATE_PREFER_BUSY_POLL, /* prefer busy-polling over
softirq processing*/
NAPI_STATE_THREADED, /* The poll is performed
inside its own thread*/
+ NAPI_STATE_SCHED_BUSY_POLL, /* Napi is currently scheduled
in busy poll mode */
@@ -1501,15 +1501,14 @@ static int napi_kthread_create(struct napi_struct *n){interr=0;-/* Create and wake up the kthread once to put it in-*TASK_INTERRUPTIBLEmodetoavoidtheblockedtask-*warningandworkwithloadavg.+/* Avoid using kthread_run() here to prevent race+*betweensoftirqandkthreadpolling.*/-n->thread=kthread_run(napi_threaded_poll,n,"napi/%s-%d",-n->dev->name,n->napi_id);+n->thread=kthread_create(napi_threaded_poll,n,"napi/%s-%d",+n->dev->name,n->napi_id);
I'm not sure this takes care of rapid:
dev_set_threaded(0)
# NAPI gets sent to sirq
dev_set_threaded(1)
since subsequent set_threaded(1) doesn't spawn the thread "afresh".
I think the race between softirq and kthread could be purely dependent
on the SCHED bit. In napi_schedule_prep(), we check if SCHED bit is
set. And we only call ____napi_schedule() when SCHED bit is not set.
In ____napi_schedule(), we either wake up kthread, or raise softirq,
never both.
So as long as we don't wake up the kthread when creating it, there
should not be a chance of race between softirq and kthread.
quoted
if (IS_ERR(n->thread)) {
err = PTR_ERR(n->thread);
- pr_err("kthread_run failed with err %d\n", err);
+ pr_err("kthread_create failed with err %d\n", err);
n->thread = NULL;
}
int work_done)
WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));
new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
+ NAPIF_STATE_SCHED_BUSY_POLL |
NAPIF_STATE_PREFER_BUSY_POLL);
/* If STATE_MISSED was set, leave STATE_SCHED set,
@@ -6971,7 +6973,10 @@ static int napi_thread_wait(struct napi_struct *napi) set_current_state(TASK_INTERRUPTIBLE); while (!kthread_should_stop() && !napi_disable_pending(napi)) {- if (test_bit(NAPI_STATE_SCHED, &napi->state)) {+ unsigned long val = READ_ONCE(napi->state);++ if (val & NAPIF_STATE_SCHED &&+ !(val & NAPIF_STATE_SCHED_BUSY_POLL)) {
Again, not protected from the napi_disable() case AFAICT.
Hmmm..... Yes. I think you are right. I missed that napi_disable()
also grabs the SCHED bit. In this case, I think we have to use the
SCHED_THREADED bit. The SCHED_BUSY_POLL bit is not enough to protect
the race between napi_disable() and napi_threaded_poll(). :(
Sorry, I missed this point when evaluating both solutions. I will have
to switch to use the SCHED_THREADED bit.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-02-26 23:11:42
On Fri, 26 Feb 2021 14:24:29 -0800 Wei Wang wrote:
quoted
I'm not sure this takes care of rapid:
dev_set_threaded(0)
# NAPI gets sent to sirq
dev_set_threaded(1)
since subsequent set_threaded(1) doesn't spawn the thread "afresh".
I think the race between softirq and kthread could be purely dependent
on the SCHED bit. In napi_schedule_prep(), we check if SCHED bit is
set. And we only call ____napi_schedule() when SCHED bit is not set.
In ____napi_schedule(), we either wake up kthread, or raise softirq,
never both.
So as long as we don't wake up the kthread when creating it, there
should not be a chance of race between softirq and kthread.
But we don't destroy the thread when dev_set_threaded(0) is called, or
make sure that it gets parked, we just clear NAPI_STATE_THREADED and
that's it.
The thread could be running long after NAPI_STATE_THREADED was cleared,
and long after it gave up NAPI_STATE_SCHED. E.g. if some heavy sirq
processing kicks in at the very moment we reenable BH.
quoted
quoted
while (!kthread_should_stop() && !napi_disable_pending(napi)) {
- if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
+ unsigned long val = READ_ONCE(napi->state);
+
+ if (val & NAPIF_STATE_SCHED &&
+ !(val & NAPIF_STATE_SCHED_BUSY_POLL)) {
Again, not protected from the napi_disable() case AFAICT.
Hmmm..... Yes. I think you are right. I missed that napi_disable()
also grabs the SCHED bit. In this case, I think we have to use the
SCHED_THREADED bit. The SCHED_BUSY_POLL bit is not enough to protect
the race between napi_disable() and napi_threaded_poll(). :(
Sorry, I missed this point when evaluating both solutions. I will have
to switch to use the SCHED_THREADED bit.
Alright, AFAICT SCHED_THREADED doesn't suffer either of the problems
I brought up here.