diff mbox

[2/5] i40e/i40evf: Add support for bulk free in Tx cleanup

Message ID 20160307173003.5682.90872.stgit@localhost.localdomain
State Accepted
Delegated to: Jeff Kirsher
Headers show

Commit Message

Alexander Duyck March 7, 2016, 5:30 p.m. UTC 
This patch enables bulk Tx clean for skbs.  In order to enable it we need
to pass the napi_budget value as that is used to determine if we are truly
running in NAPI mode or if we are simply calling the routine from netpoll
with a budget of 0.  In order to avoid adding too many more variables I
thought it best to pass the VSI directly in a fashion similar to what we do
on igb and ixgbe with the q_vector.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |   20 +++++++++++---------
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c |   20 +++++++++++---------
 2 files changed, 22 insertions(+), 18 deletions(-)

Comments

Jesse Brandeburg March 8, 2016, 7:39 p.m. UTC | #1 
Thanks Alex, one comment below.

On Mon, 7 Mar 2016 09:30:03 -0800
Alexander Duyck <aduyck@mirantis.com> wrote:
> @@ -1975,7 +1977,7 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>  	 * budget and be more aggressive about cleaning up the Tx descriptors.
>  	 */
>  	i40e_for_each_ring(ring, q_vector->tx) {
> -		if (!i40e_clean_tx_irq(ring, vsi->work_limit)) {
> +		if (!i40e_clean_tx_irq(vsi, ring, budget)) {
>  			clean_complete = false;
>  			continue;
>  		}

I'm not sure if this was a search/replace miss or if you intended it,
but I believe that limiting our transmit cleanup work in i40e_napi_poll
to budget is wrong, as transmit cleanup is so cheap compared to rx,
that we typically don't need to limit it to 64 skbs cleaned.  We can't
just have it clean unlimited numbers, so we put in a work limit that is
adjustable via ethtool and defaults to half the ring size.

So this change 
1) breaks the ethtool adjustment of tx work_limit, and
2) significantly decreases the number of transmits we will clean per
poll loop.
Alexander Duyck March 8, 2016, 7:58 p.m. UTC | #2 
On Tue, Mar 8, 2016 at 11:39 AM, Jesse Brandeburg
<jesse.brandeburg@intel.com> wrote:
> Thanks Alex, one comment below.
>
> On Mon, 7 Mar 2016 09:30:03 -0800
> Alexander Duyck <aduyck@mirantis.com> wrote:
>> @@ -1975,7 +1977,7 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
>>        * budget and be more aggressive about cleaning up the Tx descriptors.
>>        */
>>       i40e_for_each_ring(ring, q_vector->tx) {
>> -             if (!i40e_clean_tx_irq(ring, vsi->work_limit)) {
>> +             if (!i40e_clean_tx_irq(vsi, ring, budget)) {
>>                       clean_complete = false;
>>                       continue;
>>               }
>
> I'm not sure if this was a search/replace miss or if you intended it,
> but I believe that limiting our transmit cleanup work in i40e_napi_poll
> to budget is wrong, as transmit cleanup is so cheap compared to rx,
> that we typically don't need to limit it to 64 skbs cleaned.  We can't
> just have it clean unlimited numbers, so we put in a work limit that is
> adjustable via ethtool and defaults to half the ring size.
>
> So this change
> 1) breaks the ethtool adjustment of tx work_limit, and
> 2) significantly decreases the number of transmits we will clean per
> poll loop.

Please re-read the patch.  You notice I am passing the VSI as the
first argument.  That is how I am still acquiring the budget as it is
pulled from the vsi.

The value passed in the last parameter is the NAPI budget.  The NAPI
budget value is being passed as it is needed by the napi_consume_skb
call in order to determine if we were called with a budget of 0 or
not.  If we were that indicates this is actually a netpoll call, if it
is non-zero then it indicates we are a standard NAPI call.

- Alex
Bowers, AndrewX March 10, 2016, 6:38 p.m. UTC | #3 
> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Alexander Duyck
> Sent: Monday, March 07, 2016 9:30 AM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 2/5] i40e/i40evf: Add support for bulk free
> in Tx cleanup
> 
> This patch enables bulk Tx clean for skbs.  In order to enable it we need to
> pass the napi_budget value as that is used to determine if we are truly
> running in NAPI mode or if we are simply calling the routine from netpoll with
> a budget of 0.  In order to avoid adding too many more variables I thought it
> best to pass the VSI directly in a fashion similar to what we do on igb and
> ixgbe with the q_vector.
> 
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   |   20 +++++++++++---------
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c |   20 +++++++++++---------
>  2 files changed, 22 insertions(+), 18 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Driver correctly builds and passes traffic normally.
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 8fb2a966d70e..01cff073f8db 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -636,19 +636,21 @@  u32 i40e_get_tx_pending(struct i40e_ring *ring, bool in_sw)
 
 /**
  * i40e_clean_tx_irq - Reclaim resources after transmit completes
- * @tx_ring:  tx ring to clean
- * @budget:   how many cleans we're allowed
+ * @vsi: the VSI we care about
+ * @tx_ring: Tx ring to clean
+ * @napi_budget: Used to determine if we are in netpoll
  *
  * Returns true if there's any budget left (e.g. the clean is finished)
  **/
-static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
+static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
+			      struct i40e_ring *tx_ring, int napi_budget)
 {
 	u16 i = tx_ring->next_to_clean;
 	struct i40e_tx_buffer *tx_buf;
 	struct i40e_tx_desc *tx_head;
 	struct i40e_tx_desc *tx_desc;
-	unsigned int total_packets = 0;
-	unsigned int total_bytes = 0;
+	unsigned int total_bytes = 0, total_packets = 0;
+	unsigned int budget = vsi->work_limit;
 
 	tx_buf = &tx_ring->tx_bi[i];
 	tx_desc = I40E_TX_DESC(tx_ring, i);
@@ -678,7 +680,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_consume_skb_any(tx_buf->skb);
+		napi_consume_skb(tx_buf->skb, napi_budget);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
@@ -749,7 +751,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 
 		if (budget &&
 		    ((j / (WB_STRIDE + 1)) == 0) && (j != 0) &&
-		    !test_bit(__I40E_DOWN, &tx_ring->vsi->state) &&
+		    !test_bit(__I40E_DOWN, &vsi->state) &&
 		    (I40E_DESC_UNUSED(tx_ring) != tx_ring->count))
 			tx_ring->arm_wb = true;
 	}
@@ -767,7 +769,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		smp_mb();
 		if (__netif_subqueue_stopped(tx_ring->netdev,
 					     tx_ring->queue_index) &&
-		   !test_bit(__I40E_DOWN, &tx_ring->vsi->state)) {
+		   !test_bit(__I40E_DOWN, &vsi->state)) {
 			netif_wake_subqueue(tx_ring->netdev,
 					    tx_ring->queue_index);
 			++tx_ring->tx_stats.restart_queue;
@@ -1975,7 +1977,7 @@  int i40e_napi_poll(struct napi_struct *napi, int budget)
 	 * budget and be more aggressive about cleaning up the Tx descriptors.
 	 */
 	i40e_for_each_ring(ring, q_vector->tx) {
-		if (!i40e_clean_tx_irq(ring, vsi->work_limit)) {
+		if (!i40e_clean_tx_irq(vsi, ring, budget)) {
 			clean_complete = false;
 			continue;
 		}
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 839a6df62f72..9e911363c11b 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -155,19 +155,21 @@  u32 i40evf_get_tx_pending(struct i40e_ring *ring, bool in_sw)
 
 /**
  * i40e_clean_tx_irq - Reclaim resources after transmit completes
- * @tx_ring:  tx ring to clean
- * @budget:   how many cleans we're allowed
+ * @vsi: the VSI we care about
+ * @tx_ring: Tx ring to clean
+ * @napi_budget: Used to determine if we are in netpoll
  *
  * Returns true if there's any budget left (e.g. the clean is finished)
  **/
-static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
+static bool i40e_clean_tx_irq(struct i40e_vsi *vsi,
+			      struct i40e_ring *tx_ring, int napi_budget)
 {
 	u16 i = tx_ring->next_to_clean;
 	struct i40e_tx_buffer *tx_buf;
 	struct i40e_tx_desc *tx_head;
 	struct i40e_tx_desc *tx_desc;
-	unsigned int total_packets = 0;
-	unsigned int total_bytes = 0;
+	unsigned int total_bytes = 0, total_packets = 0;
+	unsigned int budget = vsi->work_limit;
 
 	tx_buf = &tx_ring->tx_bi[i];
 	tx_desc = I40E_TX_DESC(tx_ring, i);
@@ -197,7 +199,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		total_packets += tx_buf->gso_segs;
 
 		/* free the skb */
-		dev_kfree_skb_any(tx_buf->skb);
+		napi_consume_skb(tx_buf->skb, napi_budget);
 
 		/* unmap skb header data */
 		dma_unmap_single(tx_ring->dev,
@@ -267,7 +269,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 
 		if (budget &&
 		    ((j / (WB_STRIDE + 1)) == 0) && (j > 0) &&
-		    !test_bit(__I40E_DOWN, &tx_ring->vsi->state) &&
+		    !test_bit(__I40E_DOWN, &vsi->state) &&
 		    (I40E_DESC_UNUSED(tx_ring) != tx_ring->count))
 			tx_ring->arm_wb = true;
 	}
@@ -285,7 +287,7 @@  static bool i40e_clean_tx_irq(struct i40e_ring *tx_ring, int budget)
 		smp_mb();
 		if (__netif_subqueue_stopped(tx_ring->netdev,
 					     tx_ring->queue_index) &&
-		   !test_bit(__I40E_DOWN, &tx_ring->vsi->state)) {
+		   !test_bit(__I40E_DOWN, &vsi->state)) {
 			netif_wake_subqueue(tx_ring->netdev,
 					    tx_ring->queue_index);
 			++tx_ring->tx_stats.restart_queue;
@@ -1411,7 +1413,7 @@  int i40evf_napi_poll(struct napi_struct *napi, int budget)
 	 * budget and be more aggressive about cleaning up the Tx descriptors.
 	 */
 	i40e_for_each_ring(ring, q_vector->tx) {
-		if (!i40e_clean_tx_irq(ring, vsi->work_limit)) {
+		if (!i40e_clean_tx_irq(vsi, ring, budget)) {
 			clean_complete = false;
 			continue;
 		}