[PATCH net] ixgbe: napi_poll must return the work done

Currently the function ixgbe_poll() returns 0 when it clean completely
the rx rings, but this foul budget accounting in core code.
Fix this returning the actual work done, capped to weight - 1, since
the core doesn't allow to return the full budget when the driver modifies
the napi status

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 088c47c..8bebd86 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -2887,7 +2887,7 @@ int ixgbe_poll(struct napi_struct *napi, int budget)
 	if (!test_bit(__IXGBE_DOWN, &adapter->state))
 		ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector->v_idx));
 
-	return 0;
+	return min(work_done, budget - 1);
 }
 
 /**

-- 
1.8.3.1

On Wed, Jun 15, 2016 at 6:37 AM, Paolo Abeni [off-list ref] wrote:

Currently the function ixgbe_poll() returns 0 when it clean completely
the rx rings, but this foul budget accounting in core code.
Fix this returning the actual work done, capped to weight - 1, since
the core doesn't allow to return the full budget when the driver modifies
the napi status

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

I think the origin of reporting 0 was actually compatibility with some
NAPI code floating around from before the 2.6.24 kernel.

I'd be curious to know how much this is actually fouling things up.
Can you point to any specific issues it was causing?  If you end up
having to submit a v2 for any reason it might be useful if you can
provide the additional details on what actual issue it was causing.

You might also want to look at the other Intel drivers, specifically
ixgbevf and fm10k as I believe we have similar code in those drivers
as well.

Acked-by: Alexander Duyck <redacted>

On Wed, 2016-06-15 at 08:20 -0700, Alexander Duyck wrote:

On Wed, Jun 15, 2016 at 6:37 AM, Paolo Abeni [off-list ref] wrote:

quoted

Currently the function ixgbe_poll() returns 0 when it clean completely
the rx rings, but this foul budget accounting in core code.
Fix this returning the actual work done, capped to weight - 1, since
the core doesn't allow to return the full budget when the driver modifies
the napi status

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

I think the origin of reporting 0 was actually compatibility with some
NAPI code floating around from before the 2.6.24 kernel.

I'd be curious to know how much this is actually fouling things up.
Can you point to any specific issues it was causing?  

I noticed this while instrumenting the napi poll loop for another
patch. 

It's not easy to reproduce the bugged scenario, several NICs receiving a
relevant amount of traffic on napi instances scheduled on the same
softirq are needed. 

If any/some of them has the buggy poll() method, the napi_poll() loop
may process (much) more than netdev_budget packets per invocation,
possibly delaying others softirq more than needed/expected. 

The maxium delay will be no matter what capped to a couple of jiffies,
due to the time-based loop end condition, so in the worst possible
scenario (most probably not a real thing), this adds a latency of 2
jiffies - <time required to process netdev_budget packets> (~1.8ms on
recent h/w with HZ==1000).

If you end up
having to submit a v2 for any reason it might be useful if you can
provide the additional details on what actual issue it was causing.

You might also want to look at the other Intel drivers, specifically
ixgbevf and fm10k as I believe we have similar code in those drivers
as well.

Thank you for the head-up. I need to get an hand on that h/w, first!

Paolo

Acked-by: Alexander Duyck <redacted>

On Wed, Jun 15, 2016 at 6:37 AM, Paolo Abeni [off-list ref] wrote:

Reviewed-by: Venkatesh Srinivas <redacted>

The same bit of code appears in fm10k and i40e/i40evf. ixgb appears to
correctly return work_done.

ixgbe_poll also appears to return an (minor) incorrect work_done in
another case, BTW. It divides its
budget between Rx rings associated with a vector. If any ring exceeds
its share of the budget, ixgbe_poll
claims to have consumed the full budget, even if a full budget of
frames was not received in a single
pass.

-- vs;

On Wed, 2016-06-15 at 09:34 -0700, Venkatesh Srinivas wrote:

On Wed, Jun 15, 2016 at 6:37 AM, Paolo Abeni [off-list ref]
wrote:

quoted

Currently the function ixgbe_poll() returns 0 when it clean
completely
the rx rings, but this foul budget accounting in core code.
Fix this returning the actual work done, capped to weight - 1,
since
the core doesn't allow to return the full budget when the driver
modifies
the napi status

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 088c47c..8bebd86 100644

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -2887,7 +2887,7 @@ int ixgbe_poll(struct napi_struct *napi, int

budget)
        if (!test_bit(__IXGBE_DOWN, &adapter->state))
                ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector-

quoted

v_idx));

-       return 0;
+       return min(work_done, budget - 1);
 }

 /**

Reviewed-by: Venkatesh Srinivas <redacted>

The same bit of code appears in fm10k and i40e/i40evf. ixgb appears
to
correctly return work_done.

ixgbe_poll also appears to return an (minor) incorrect work_done in
another case, BTW. It divides its
budget between Rx rings associated with a vector. If any ring exceeds
its share of the budget, ixgbe_poll
claims to have consumed the full budget, even if a full budget of
frames was not received in a single
pass.

-- vs;

I can submit a patch for fm10k.

Thanks,
Jake

On Wed, 2016-06-15 at 09:34 -0700, Venkatesh Srinivas wrote:

On Wed, Jun 15, 2016 at 6:37 AM, Paolo Abeni [off-list ref]
wrote:

quoted

Currently the function ixgbe_poll() returns 0 when it clean
completely
the rx rings, but this foul budget accounting in core code.
Fix this returning the actual work done, capped to weight - 1,
since
the core doesn't allow to return the full budget when the driver
modifies
the napi status

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 088c47c..8bebd86 100644

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -2887,7 +2887,7 @@ int ixgbe_poll(struct napi_struct *napi, int

budget)
        if (!test_bit(__IXGBE_DOWN, &adapter->state))
                ixgbe_irq_enable_queues(adapter, BIT_ULL(q_vector-

quoted

v_idx));

-       return 0;
+       return min(work_done, budget - 1);
 }

 /**

Reviewed-by: Venkatesh Srinivas <redacted>

The same bit of code appears in fm10k and i40e/i40evf. ixgb appears
to
correctly return work_done.

ixgbe_poll also appears to return an (minor) incorrect work_done in
another case, BTW. It divides its
budget between Rx rings associated with a vector. If any ring exceeds
its share of the budget, ixgbe_poll
claims to have consumed the full budget, even if a full budget of
frames was not received in a single
pass.

-- vs;

For the record, I couldn't find any documentation on this in
Documentatino/networking, or as a function header. Where would be the
best place to document the expectations of the napi core? I'd like to
submit a patch so that future it will be easier to determine what a new
driver should do (without just blindly copying from other drivers as
has caused these so far).

Thanks,
Jake

On Wed, 2016-06-15 at 09:34 -0700, Venkatesh Srinivas wrote:

Reviewed-by: Venkatesh Srinivas <redacted>

The same bit of code appears in fm10k and i40e/i40evf. ixgb appears
to
correctly return work_done.

ixgbe_poll also appears to return an (minor) incorrect work_done in
another case, BTW. It divides its
budget between Rx rings associated with a vector. If any ring exceeds
its share of the budget, ixgbe_poll
claims to have consumed the full budget, even if a full budget of
frames was not received in a single
pass.

So the correct return here would also be min between work_done and
budget, right? ie: we should still return the total work done instead
of the budget...?

Thanks,
Jake

-- vs;