[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Jacob Keller]

When fm10k_poll fully cleans rings it returns 0. This is incorrect as it
messes up the budget accounting in the core napi code. Fix this by
returning actual work done, capped at budget - 1 since the core doesn't
expect a return of the full budget when the driver modifies the napi
status.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Venkatesh Srinivas <venkateshs@google.com>
Cc: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 682a372bdb20..79d5093d83d1 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1484,7 +1484,7 @@ static int fm10k_poll(struct napi_struct *napi, int budget)
 	/* re-enable the q_vector */
 	fm10k_qv_enable(q_vector);
 
-	return 0;
+	return min(work_done, budget - 1);
 }
 
 /**
-- 
2.9.0.rc1.405.g81f467e

[Intel-wired-lan] [PATCH 2/2] fm10k: return smaller of actual work done or budget in fm10k_poll

[Jacob Keller]

Correct a slight issue with fm10k_poll returning the full budget even if
it didn't fully exhaust it. This occurs if a single ring exhausts its
per-ring portion of the budget but the other rings do not. Return the
actual amount we used capped at budget to avoid issues with napi core.

Change-Id: Ie84de029a62adcfa8af49ccb2dc0b4d9f378ee71
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Venkatesh Srinivas <venkateshs@google.com>
Cc: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/fm10k/fm10k_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 79d5093d83d1..c2c7f2ef2152 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -1474,9 +1474,9 @@ static int fm10k_poll(struct napi_struct *napi, int budget)
 			clean_complete = false;
 	}
 
-	/* If all work not completed, return budget and keep polling */
+	/* If all work not completed, return work done and keep polling */
 	if (!clean_complete)
-		return budget;
+		return min(work_done, budget);
 
 	/* all work done, exit the polling mode */
 	napi_complete_done(napi, work_done);
-- 
2.9.0.rc1.405.g81f467e

[Intel-wired-lan] [PATCH 2/2] fm10k: return smaller of actual work done or budget in fm10k_poll

[Keller, Jacob E]

> -----Original Message-----
> From: Keller, Jacob E
> Sent: Monday, June 20, 2016 10:40 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Paolo Abeni
> <pabeni@redhat.com>; Venkatesh Srinivas <venkateshs@google.com>;
> Alexander Duyck <aduyck@mirantis.com>
> Subject: [PATCH 2/2] fm10k: return smaller of actual work done or budget in
> fm10k_poll
> 
> Correct a slight issue with fm10k_poll returning the full budget even if
> it didn't fully exhaust it. This occurs if a single ring exhausts its
> per-ring portion of the budget but the other rings do not. Return the
> actual amount we used capped at budget to avoid issues with napi core.
> 
> Change-Id: Ie84de029a62adcfa8af49ccb2dc0b4d9f378ee71
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Venkatesh Srinivas <venkateshs@google.com>
> Cc: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> index 79d5093d83d1..c2c7f2ef2152 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> @@ -1474,9 +1474,9 @@ static int fm10k_poll(struct napi_struct *napi, int
> budget)
>  			clean_complete = false;
>  	}
> 
> -	/* If all work not completed, return budget and keep polling */
> +	/* If all work not completed, return work done and keep polling */
>  	if (!clean_complete)
> -		return budget;
> +		return min(work_done, budget);
> 

This patch potentially appears to be caused Tx hangs in the napi routines in the stack, when under heavy traffic pressure. It appears that other drivers (i40e, and ixgbe) don't do this either, so I am not sure which is correct...

>  	/* all work done, exit the polling mode */
>  	napi_complete_done(napi, work_done);
> --
> 2.9.0.rc1.405.g81f467e

[Intel-wired-lan] [PATCH 2/2] fm10k: return smaller of actual work done or budget in fm10k_poll

[Keller, Jacob E]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Keller, Jacob E
> Sent: Wednesday, August 10, 2016 12:49 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Duyck, Alexander H
> <alexander.h.duyck@intel.com>
> Cc: Paolo Abeni <pabeni@redhat.com>; Venkatesh Srinivas
> <venkateshs@google.com>
> Subject: Re: [Intel-wired-lan] [PATCH 2/2] fm10k: return smaller of actual
> work done or budget in fm10k_poll
> 
> > -----Original Message-----
> > From: Keller, Jacob E
> > Sent: Monday, June 20, 2016 10:40 AM
> > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Paolo Abeni
> > <pabeni@redhat.com>; Venkatesh Srinivas <venkateshs@google.com>;
> > Alexander Duyck <aduyck@mirantis.com>
> > Subject: [PATCH 2/2] fm10k: return smaller of actual work done or budget
> in
> > fm10k_poll
> >
> > Correct a slight issue with fm10k_poll returning the full budget even if
> > it didn't fully exhaust it. This occurs if a single ring exhausts its
> > per-ring portion of the budget but the other rings do not. Return the
> > actual amount we used capped at budget to avoid issues with napi core.
> >
> > Change-Id: Ie84de029a62adcfa8af49ccb2dc0b4d9f378ee71
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Venkatesh Srinivas <venkateshs@google.com>
> > Cc: Alexander Duyck <aduyck@mirantis.com>
> > ---
> >  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > index 79d5093d83d1..c2c7f2ef2152 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > @@ -1474,9 +1474,9 @@ static int fm10k_poll(struct napi_struct *napi,
> int
> > budget)
> >  			clean_complete = false;
> >  	}
> >
> > -	/* If all work not completed, return budget and keep polling */
> > +	/* If all work not completed, return work done and keep polling */
> >  	if (!clean_complete)
> > -		return budget;
> > +		return min(work_done, budget);
> >
> 
> This patch potentially appears to be caused Tx hangs in the napi routines in
> the stack, when under heavy traffic pressure. It appears that other drivers
> (i40e, and ixgbe) don't do this either, so I am not sure which is correct...
> 

This patch causes false Tx hangs due to potentially returning 0 when work_done is 0 but the Tx rings aren't clean. After some discussion with Alex I believe that the patch is just wrong and should be dropped. Jeff, can you drop this patch from the queue? The other related patch is actually correct and should remain.

Thanks,
Jake

[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Paolo Abeni]

On Mon, 2016-06-20 at 10:39 -0700, Jacob Keller wrote:
> When fm10k_poll fully cleans rings it returns 0. This is incorrect as it
> messes up the budget accounting in the core napi code. Fix this by
> returning actual work done, capped at budget - 1 since the core doesn't
> expect a return of the full budget when the driver modifies the napi
> status.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Venkatesh Srinivas <venkateshs@google.com>
> Cc: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Paolo Abeni <pabeni@redhat.com>

[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Keller, Jacob E]

> -----Original Message-----
> From: Keller, Jacob E
> Sent: Monday, June 20, 2016 10:40 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Paolo Abeni
> <pabeni@redhat.com>; Venkatesh Srinivas <venkateshs@google.com>;
> Alexander Duyck <aduyck@mirantis.com>
> Subject: [PATCH 1/2] fm10k: napi polling routine must return actual work
> done
> 
> When fm10k_poll fully cleans rings it returns 0. This is incorrect as it
> messes up the budget accounting in the core napi code. Fix this by
> returning actual work done, capped at budget - 1 since the core doesn't
> expect a return of the full budget when the driver modifies the napi
> status.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Venkatesh Srinivas <venkateshs@google.com>
> Cc: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> index 682a372bdb20..79d5093d83d1 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> @@ -1484,7 +1484,7 @@ static int fm10k_poll(struct napi_struct *napi, int
> budget)
>  	/* re-enable the q_vector */
>  	fm10k_qv_enable(q_vector);
> 
> -	return 0;
> +	return min(work_done, budget - 1);


This patch potentially appears to be caused Tx hangs in the napi routines in the stack, when under heavy traffic pressure. It appears that other drivers (i40e, and ixgbe) don't do this either, so I am not sure which is correct...

>  }
> 
>  /**
> --
> 2.9.0.rc1.405.g81f467e

[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Keller, Jacob E]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Keller, Jacob E
> Sent: Wednesday, August 10, 2016 12:50 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>; Duyck, Alexander H
> <alexander.h.duyck@intel.com>
> Cc: Paolo Abeni <pabeni@redhat.com>; Venkatesh Srinivas
> <venkateshs@google.com>
> Subject: Re: [Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must
> return actual work done
> 
> 
> 
> > -----Original Message-----
> > From: Keller, Jacob E
> > Sent: Monday, June 20, 2016 10:40 AM
> > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Paolo Abeni
> > <pabeni@redhat.com>; Venkatesh Srinivas <venkateshs@google.com>;
> > Alexander Duyck <aduyck@mirantis.com>
> > Subject: [PATCH 1/2] fm10k: napi polling routine must return actual work
> > done
> >
> > When fm10k_poll fully cleans rings it returns 0. This is incorrect as it
> > messes up the budget accounting in the core napi code. Fix this by
> > returning actual work done, capped at budget - 1 since the core doesn't
> > expect a return of the full budget when the driver modifies the napi
> > status.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Venkatesh Srinivas <venkateshs@google.com>
> > Cc: Alexander Duyck <aduyck@mirantis.com>
> > ---
> >  drivers/net/ethernet/intel/fm10k/fm10k_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > index 682a372bdb20..79d5093d83d1 100644
> > --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > @@ -1484,7 +1484,7 @@ static int fm10k_poll(struct napi_struct *napi,
> int
> > budget)
> >  	/* re-enable the q_vector */
> >  	fm10k_qv_enable(q_vector);
> >
> > -	return 0;
> > +	return min(work_done, budget - 1);
> 
> 
> This patch potentially appears to be caused Tx hangs in the napi routines in
> the stack, when under heavy traffic pressure. It appears that other drivers
> (i40e, and ixgbe) don't do this either, so I am not sure which is correct...
> 

This patch is fine and should remain.

Thanks,
Jake

[Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

[Singh, Krishneil K]

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jacob Keller
Sent: Monday, June 20, 2016 10:40 AM
To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
Cc: Paolo Abeni <pabeni@redhat.com>; Alexander Duyck <aduyck@mirantis.com>; Venkatesh Srinivas <venkateshs@google.com>
Subject: [Intel-wired-lan] [PATCH 1/2] fm10k: napi polling routine must return actual work done

When fm10k_poll fully cleans rings it returns 0. This is incorrect as it messes up the budget accounting in the core napi code. Fix this by returning actual work done, capped at budget - 1 since the core doesn't expect a return of the full budget when the driver modifies the napi status.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Venkatesh Srinivas <venkateshs@google.com>
Cc: Alexander Duyck <aduyck@mirantis.com>
---
 
Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>