Hello.
I've raised this here [1] first, but was suggested to engage igb devs,
so here we are.
I'm experiencing the following woes while using netconsole regularly:
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-04-06 18:48:07
On Tue, 6 Apr 2021 14:36:19 +0200 Oleksandr Natalenko wrote:
Hello.
I've raised this here [1] first, but was suggested to engage igb devs,
so here we are.
I'm experiencing the following woes while using netconsole regularly:
Looks like igb_clean_tx_irq() should not return true if budget is 0,
ever, otherwise we risk hitting the min(work, budget - 1) which may
go negative.
So something like this?
@@ -8157,7 +8157,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector, int napi_budget)tx_ring->queue_index);/* we are about to reset, no point in enabling stuff */-returntrue;+gotoout;}}
Hello.
On Tue, Apr 06, 2021 at 11:48:02AM -0700, Jakub Kicinski wrote:
quoted hunk
On Tue, 6 Apr 2021 14:36:19 +0200 Oleksandr Natalenko wrote:
quoted
Hello.
I've raised this here [1] first, but was suggested to engage igb devs,
so here we are.
I'm experiencing the following woes while using netconsole regularly:
Looks like igb_clean_tx_irq() should not return true if budget is 0,
ever, otherwise we risk hitting the min(work, budget - 1) which may
go negative.
So something like this?
@@ -8157,7 +8157,7 @@ static bool igb_clean_tx_irq(struct igb_q_vector *q_vector, int napi_budget)tx_ring->queue_index);/* we are about to reset, no point in enabling stuff */-returntrue;+gotoout;}}
@@ -7980,7 +7980,7 @@ static int igb_poll(struct napi_struct *napi, int budget)structigb_q_vector*q_vector=container_of(napi,structigb_q_vector,napi);-boolclean_complete=true;+boolclean_complete=q_vector->tx.ring||q_vector->rx.ring;intwork_done=0;#ifdef CONFIG_IGB_DCA
It might make sense to just cast the work_done as a unsigned int, and
then on the end of igb_poll use:
return min_t(unsigned int, work_done, budget - 1);
@@ -7980,7 +7980,7 @@ static int igb_poll(struct napi_struct *napi, int budget)structigb_q_vector*q_vector=container_of(napi,structigb_q_vector,napi);-boolclean_complete=true;+boolclean_complete=q_vector->tx.ring||q_vector->rx.ring;intwork_done=0;#ifdef CONFIG_IGB_DCA
It might make sense to just cast the work_done as a unsigned int, and
then on the end of igb_poll use:
return min_t(unsigned int, work_done, budget - 1);
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
@@ -7980,7 +7980,7 @@ static int igb_poll(struct napi_struct *napi, int budget)structigb_q_vector*q_vector=container_of(napi,structigb_q_vector,napi);-boolclean_complete=true;+boolclean_complete=q_vector->tx.ring||q_vector->rx.ring;intwork_done=0;#ifdef CONFIG_IGB_DCA
It might make sense to just cast the work_done as a unsigned int, and
then on the end of igb_poll use:
return min_t(unsigned int, work_done, budget - 1);
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
@@ -7980,7 +7980,7 @@ static int igb_poll(struct napi_struct *napi, int budget)structigb_q_vector*q_vector=container_of(napi,structigb_q_vector,napi);-boolclean_complete=true;+boolclean_complete=q_vector->tx.ring||q_vector->rx.ring;intwork_done=0;#ifdef CONFIG_IGB_DCA
It might make sense to just cast the work_done as a unsigned int, and
then on the end of igb_poll use:
return min_t(unsigned int, work_done, budget - 1);
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Thanks.
--
Oleksandr Natalenko (post-factum)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-04-23 22:58:52
On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
quoted
On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski [off-list ref] wrote:
quoted
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Alex's suggestion is probably best.
I'm not aware of the fix being posted. Perhaps you could take over and
post the patch if Intel doesn't chime in?
Hello.
On Fri, Apr 23, 2021 at 03:58:36PM -0700, Jakub Kicinski wrote:
On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
quoted
On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
quoted
On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski [off-list ref] wrote:
quoted
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Alex's suggestion is probably best.
I'm not aware of the fix being posted. Perhaps you could take over and
post the patch if Intel doesn't chime in?
From: Alexander Duyck <hidden> Date: 2021-04-26 15:28:56
On Sun, Apr 25, 2021 at 11:47 PM Oleksandr Natalenko
[off-list ref] wrote:
Hello.
On Fri, Apr 23, 2021 at 03:58:36PM -0700, Jakub Kicinski wrote:
quoted
On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
quoted
On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
quoted
On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski [off-list ref] wrote:
quoted
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Alex's suggestion is probably best.
I'm not aware of the fix being posted. Perhaps you could take over and
post the patch if Intel doesn't chime in?
Actually a better way to go would be to probably just initialize
"clean_complete = !!budget". With that we don't have it messing with
the interrupt enables which would probably be a better behavior.
On Sun, Apr 25, 2021 at 11:47 PM Oleksandr Natalenko
[off-list ref] wrote:
quoted
Hello.
On Fri, Apr 23, 2021 at 03:58:36PM -0700, Jakub Kicinski wrote:
quoted
On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
quoted
On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
quoted
On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski [off-list ref] wrote:
quoted
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Alex's suggestion is probably best.
I'm not aware of the fix being posted. Perhaps you could take over and
post the patch if Intel doesn't chime in?
Actually a better way to go would be to probably just initialize
"clean_complete = !!budget". With that we don't have it messing with
the interrupt enables which would probably be a better behavior.
Thanks guys for the suggestions here! Finally got some time for
this, so here is the patch I'm going to queue shortly.
From ffd24e90d688ee347ab051266bfc7fca00324a68 Mon Sep 17 00:00:00 2001
From: Jesse Brandeburg <redacted>
Date: Thu, 6 May 2021 14:41:11 -0700
Subject: [PATCH net] igb: fix netpoll exit with traffic
To: netdev,
Oleksandr Natalenko [off-list ref]
Cc: Jakub Kicinski <kuba@kernel.org>, LKML <redacted>, "Brandeburg, Jesse" <redacted>, "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, intel-wired-lan <redacted>, Alexander Duyck <redacted>
Oleksandr brought a bug report where netpoll causes trace messages in
the log on igb.
[22038.710800] ------------[ cut here ]------------
[22038.710801] igb_poll+0x0/0x1440 [igb] exceeded budget in poll
[22038.710802] WARNING: CPU: 12 PID: 40362 at net/core/netpoll.c:155 netpoll_poll_dev+0x18a/0x1a0
After some discussion and debug from the list, it was deemed that the
right thing to do is initialize the clean_complete variable to false
when the "netpoll mode" of passing a zero budget is used.
This logic should be sane and not risky because the only time budget
should be zero on entry is netpoll. Change includes a small refactor
of local variable assignments to clean up the look.
Fixes: 16eb8815c235 ("igb: Refactor clean_rx_irq to reduce overhead and improve performance")
Reported-by: Oleksandr Natalenko <redacted>
Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Jesse Brandeburg <redacted>
---
Compile tested ONLY, but functionally it should be exactly the same for
all cases except when budget is zero on entry, which will hopefully fix
the bug.
---
drivers/net/ethernet/intel/igb/igb_main.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -7991,12 +7991,16 @@ static void igb_ring_irq_enable(struct igb_q_vector *q_vector)**/staticintigb_poll(structnapi_struct*napi,intbudget){-structigb_q_vector*q_vector=container_of(napi,-structigb_q_vector,-napi);-boolclean_complete=true;+structigb_q_vector*q_vector;+boolclean_complete;intwork_done=0;+/* if budget is zero, we have a special case for netconsole, so+*makesuretosetclean_completetofalseinthatcase.+*/+clean_complete=!!budget;++q_vector=container_of(napi,structigb_q_vector,napi);#ifdef CONFIG_IGB_DCAif(q_vector->adapter->flags&IGB_FLAG_DCA_ENABLED)igb_update_dca(q_vector);
From: Alexander Duyck <hidden> Date: 2021-05-07 00:38:35
On Thu, May 6, 2021 at 4:32 PM Jesse Brandeburg
[off-list ref] wrote:
quoted hunk
Alexander Duyck wrote:
quoted
On Sun, Apr 25, 2021 at 11:47 PM Oleksandr Natalenko
[off-list ref] wrote:
quoted
Hello.
On Fri, Apr 23, 2021 at 03:58:36PM -0700, Jakub Kicinski wrote:
quoted
On Fri, 23 Apr 2021 10:19:44 +0200 Oleksandr Natalenko wrote:
quoted
On Wed, Apr 07, 2021 at 04:06:29PM -0700, Alexander Duyck wrote:
quoted
On Wed, Apr 7, 2021 at 11:07 AM Jakub Kicinski [off-list ref] wrote:
quoted
Sure, that's simplest. I wasn't sure something is supposed to prevent
this condition or if it's okay to cover it up.
I'm pretty sure it is okay to cover it up. In this case the "budget -
1" is supposed to be the upper limit on what can be reported. I think
it was assuming an unsigned value anyway.
Another alternative would be to default clean_complete to !!budget.
Then if budget is 0 clean_complete would always return false.
So, among all the variants, which one to try? Or there was a separate
patch sent to address this?
Alex's suggestion is probably best.
I'm not aware of the fix being posted. Perhaps you could take over and
post the patch if Intel doesn't chime in?
Actually a better way to go would be to probably just initialize
"clean_complete = !!budget". With that we don't have it messing with
the interrupt enables which would probably be a better behavior.
Thanks guys for the suggestions here! Finally got some time for
this, so here is the patch I'm going to queue shortly.
From ffd24e90d688ee347ab051266bfc7fca00324a68 Mon Sep 17 00:00:00 2001
From: Jesse Brandeburg <redacted>
Date: Thu, 6 May 2021 14:41:11 -0700
Subject: [PATCH net] igb: fix netpoll exit with traffic
To: netdev,
Oleksandr Natalenko [off-list ref]
Cc: Jakub Kicinski <kuba@kernel.org>, LKML <redacted>, "Brandeburg, Jesse" <redacted>, "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, intel-wired-lan <redacted>, Alexander Duyck <redacted>
Oleksandr brought a bug report where netpoll causes trace messages in
the log on igb.
[22038.710800] ------------[ cut here ]------------
[22038.710801] igb_poll+0x0/0x1440 [igb] exceeded budget in poll
[22038.710802] WARNING: CPU: 12 PID: 40362 at net/core/netpoll.c:155 netpoll_poll_dev+0x18a/0x1a0
After some discussion and debug from the list, it was deemed that the
right thing to do is initialize the clean_complete variable to false
when the "netpoll mode" of passing a zero budget is used.
This logic should be sane and not risky because the only time budget
should be zero on entry is netpoll. Change includes a small refactor
of local variable assignments to clean up the look.
Fixes: 16eb8815c235 ("igb: Refactor clean_rx_irq to reduce overhead and improve performance")
Reported-by: Oleksandr Natalenko <redacted>
Suggested-by: Alexander Duyck <redacted>
Signed-off-by: Jesse Brandeburg <redacted>
---
Compile tested ONLY, but functionally it should be exactly the same for
all cases except when budget is zero on entry, which will hopefully fix
the bug.
---
drivers/net/ethernet/intel/igb/igb_main.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -7991,12 +7991,16 @@ static void igb_ring_irq_enable(struct igb_q_vector *q_vector)**/staticintigb_poll(structnapi_struct*napi,intbudget){-structigb_q_vector*q_vector=container_of(napi,-structigb_q_vector,-napi);-boolclean_complete=true;+structigb_q_vector*q_vector;+boolclean_complete;intwork_done=0;+/* if budget is zero, we have a special case for netconsole, so+*makesuretosetclean_completetofalseinthatcase.+*/+clean_complete=!!budget;++q_vector=container_of(napi,structigb_q_vector,napi);#ifdef CONFIG_IGB_DCAif(q_vector->adapter->flags&IGB_FLAG_DCA_ENABLED)igb_update_dca(q_vector);
I'm not a big fan of moving the q_vector init as a part of this patch
since it just means more backport work.
That said the change itself should be harmless so I am good with it either way.
Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>