[PATCH net 0/2] fix napi return value

[PATCH net 0/2] fix napi return value

[Govindarajulu Varadarajan]

Since d75b1ade567ffab ("net: less interrupt masking in NAPI") we should return
budget for napi repoll.

In enic, we return 0 when busy_poll is happening. Fix this by returning budget.

Looks like bnx2x needs this fix as well. On bnx2x I performed compile
test only. I do not have hardware. 

Govindarajulu Varadarajan (2):
  enic: fix rx napi poll return value
  bnx2x: fix napi poll return value for repoll

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 4 ++--
 drivers/net/ethernet/cisco/enic/enic_main.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.2.2

[PATCH net 1/2] enic: fix rx napi poll return value

[Govindarajulu Varadarajan]

With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll
is done only when work_done == budget. When we are in busy_poll we return 0 in
napi_poll. We should return budget.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 drivers/net/ethernet/cisco/enic/enic_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index b29e027..e356afa 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1335,7 +1335,7 @@ static int enic_poll_msix_rq(struct napi_struct *napi, int budget)
 	int err;
 
 	if (!enic_poll_lock_napi(&enic->rq[rq]))
-		return work_done;
+		return budget;
 	/* Service RQ
 	 */
 
-- 
2.2.2

[PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Govindarajulu Varadarajan]

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(-)

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;
 			}
 		}
 
-- 
2.2.2

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Eric Dumazet]

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.

>  			}
>  		}
>  

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Eric Dumazet]

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()

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Govindarajulu Varadarajan]

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()

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Govindarajulu Varadarajan]

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

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[Eric Dumazet]

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.

Re: [PATCH net 1/2] enic: fix rx napi poll return value

[David Miller]

From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Tue, 20 Jan 2015 18:46:15 +0530

> With the commit d75b1ade567ffab ("net: less interrupt masking in NAPI") napi repoll
> is done only when work_done == budget. When we are in busy_poll we return 0 in
> napi_poll. We should return budget.
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>

Applied.

Re: [PATCH net 2/2] bnx2x: fix napi poll return value for repoll

[David Miller]

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.