| Message ID | 1421759776-376-3-git-send-email-_govind@gmx.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2015-01-20 at 18:46 +0530, Govindarajulu Varadarajan wrote: > With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll > is done only when work_done == budget. When in busy_poll is we return 0 in > napi_poll. We should return budget. Also do not return workdone > budget. > I am not sure. > Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com> > --- > drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c > index 1d1147c..ebcbe92 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c > @@ -3175,7 +3175,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) > } > #endif > if (!bnx2x_fp_lock_napi(fp)) > - return work_done; > + return budget; For this one I am not sure. Busy poll is not supposed to drain the whole queue. > > for_each_cos_in_tx_queue(fp, cos) > if (bnx2x_tx_queue_has_work(fp->txdata_ptr[cos])) > @@ -3187,7 +3187,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) > /* must not complete if we consumed full budget */ > if (work_done >= budget) { > bnx2x_fp_unlock_napi(fp); > - break; > + return budget; This one looks fine. > } > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-01-20 at 06:51 -0800, Eric Dumazet wrote: > > > > for_each_cos_in_tx_queue(fp, cos) > > if (bnx2x_tx_queue_has_work(fp->txdata_ptr[cos])) > > @@ -3187,7 +3187,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) > > /* must not complete if we consumed full budget */ > > if (work_done >= budget) { > > bnx2x_fp_unlock_napi(fp); > > - break; > > + return budget; > > This one looks fine. But its not necessary, as here budget == work_done. (work_done > budget) would be a bug from bnx2x_rx_int() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 20 Jan 2015, Eric Dumazet wrote: > On Tue, 2015-01-20 at 18:46 +0530, Govindarajulu Varadarajan wrote: >> With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll >> is done only when work_done == budget. When in busy_poll is we return 0 in >> napi_poll. We should return budget. Also do not return workdone > budget. >> > > I am not sure. > This is based on f41281d02f8b94e136f78cb1b6a5d78182c222bd & 9dfa9a27b620640322588df399eb8f624b48d877 I do not know about bnx2x, but in enic driver, when busy_poll is enables rq clean is not happening. This is because, in napi_poll() when work_done < budget, we do not repoll. At this point, enic has disables rq intr and has not called napi_complete. Driver assumes that napi will repoll. Which does not happen. Lot of drivers I have checked return full budget if they want to repoll. eg. mlx4_en_poll_rx_cq() -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 20 Jan 2015, Eric Dumazet wrote: > On Tue, 2015-01-20 at 06:51 -0800, Eric Dumazet wrote: > >>> >>> for_each_cos_in_tx_queue(fp, cos) >>> if (bnx2x_tx_queue_has_work(fp->txdata_ptr[cos])) >>> @@ -3187,7 +3187,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) >>> /* must not complete if we consumed full budget */ >>> if (work_done >= budget) { >>> bnx2x_fp_unlock_napi(fp); >>> - break; >>> + return budget; >> >> This one looks fine. > > But its not necessary, as here budget == work_done. > > (work_done > budget) would be a bug from bnx2x_rx_int() > Yes, I missed that one. This change here is not needed. Thanks -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-01-20 at 20:54 +0530, Govindarajulu Varadarajan wrote: > On Tue, 20 Jan 2015, Eric Dumazet wrote: > > > On Tue, 2015-01-20 at 18:46 +0530, Govindarajulu Varadarajan wrote: > >> With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll > >> is done only when work_done == budget. When in busy_poll is we return 0 in > >> napi_poll. We should return budget. Also do not return workdone > budget. > >> > > > > I am not sure. > > > > This is based on f41281d02f8b94e136f78cb1b6a5d78182c222bd & > 9dfa9a27b620640322588df399eb8f624b48d877 > > I do not know about bnx2x, but in enic driver, when busy_poll is enables > rq clean is not happening. This is because, in napi_poll() when > work_done < budget, we do not repoll. At this point, enic has disables rq intr > and has not called napi_complete. Driver assumes that napi will repoll. > Which does not happen. > > Lot of drivers I have checked return full budget if they want to repoll. > eg. mlx4_en_poll_rx_cq() I was referring to the "workdone > budget" condition. But yes, the fix about busy poll seems needed. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Govindarajulu Varadarajan <_govind@gmx.com> Date: Tue, 20 Jan 2015 18:46:16 +0530 > With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll > is done only when work_done == budget. When in busy_poll is we return 0 in > napi_poll. We should return budget. Also do not return workdone > budget. > > Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com> This seems like it needs some adjustments, please make those corrections and respin this patch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index 1d1147c..ebcbe92 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c @@ -3175,7 +3175,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) } #endif if (!bnx2x_fp_lock_napi(fp)) - return work_done; + return budget; for_each_cos_in_tx_queue(fp, cos) if (bnx2x_tx_queue_has_work(fp->txdata_ptr[cos])) @@ -3187,7 +3187,7 @@ static int bnx2x_poll(struct napi_struct *napi, int budget) /* must not complete if we consumed full budget */ if (work_done >= budget) { bnx2x_fp_unlock_napi(fp); - break; + return budget; } }
With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll is done only when work_done == budget. When in busy_poll is we return 0 in napi_poll. We should return budget. Also do not return workdone > budget. Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)