On 10/21/2015 09:37 AM, Lan Tianyu wrote:
When transmit a package, the end transmit desc of package
indicates whether package is sent already. Current code records
the end desc's pointer in the next_to_watch of struct tx buffer.
This code will be broken if shifting desc ring after migration.
The pointer will be invalid. This patch is to replace recording
pointer with recording the desc number of the package and find
the end decs via the first desc and desc number.
Signed-off-by: Lan Tianyu <tianyu....@intel.com>
---
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 1 +
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 19 ++++++++++++++++---
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
index 775d089..c823616 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf.h
@@ -54,6 +54,7 @@
*/
struct ixgbevf_tx_buffer {
union ixgbe_adv_tx_desc *next_to_watch;
+ u16 desc_num;
unsigned long time_stamp;
struct sk_buff *skb;
unsigned int bytecount;
So if you can't use next_to_watch why is it left in here? Also you
might want to take a look at moving desc_num to a different spot in the
buffer as you are leaving a 6 byte hole in the descriptor.
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 4446916..056841c 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -210,6 +210,7 @@ static void ixgbevf_unmap_and_free_tx_resource(struct
ixgbevf_ring *tx_ring,
DMA_TO_DEVICE);
}
tx_buffer->next_to_watch = NULL;
+ tx_buffer->desc_num = 0;
tx_buffer->skb = NULL;
dma_unmap_len_set(tx_buffer, len, 0);
This opens up a race condition. If you have a descriptor ready to be
cleaned at offset 0 what is to prevent you from just running through the
ring? You likely need to find a descriptor number that cannot be valid
to use here.
/* tx_buffer must be completely set up in the transmit path */
@@ -295,7 +296,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector
*q_vector,
union ixgbe_adv_tx_desc *tx_desc;
unsigned int total_bytes = 0, total_packets = 0;
unsigned int budget = tx_ring->count / 2;
- unsigned int i = tx_ring->next_to_clean;
+ int i, watch_index;
Where is i being initialized? It was here but you removed it. Are you
using i without initializing it?
if (test_bit(__IXGBEVF_DOWN, &adapter->state))
return true;
@@ -305,9 +306,17 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector
*q_vector,
i -= tx_ring->count;
do {
- union ixgbe_adv_tx_desc *eop_desc = tx_buffer->next_to_watch;
+ union ixgbe_adv_tx_desc *eop_desc;
+
+ if (!tx_buffer->desc_num)
+ break;
+
+ if (i + tx_buffer->desc_num >= 0)
+ watch_index = i + tx_buffer->desc_num;
+ else
+ watch_index = i + tx_ring->count + tx_buffer->desc_num;
- /* if next_to_watch is not set then there is no work pending */
+ eop_desc = IXGBEVF_TX_DESC(tx_ring, watch_index);
if (!eop_desc)
break;
So I don't see how this isn't triggering Tx hangs. I suspect for the
simple ping case desc_num will often be 0. The fact is there are many
cases where first and tx_buffer_info are the same descriptor.
@@ -320,6 +329,7 @@ static bool ixgbevf_clean_tx_irq(struct ixgbevf_q_vector
*q_vector,
/* clear next_to_watch to prevent false hangs */
tx_buffer->next_to_watch = NULL;
+ tx_buffer->desc_num = 0;
/* update the statistics for this packet */
total_bytes += tx_buffer->bytecount;
You cannot use 0 because 0 is a valid number. You are using it as a
look-ahead currently and there are cases where i is the eop_desc index.
@@ -3457,6 +3467,7 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring,
u32 tx_flags = first->tx_flags;
__le32 cmd_type;
u16 i = tx_ring->next_to_use;
+ u16 start;
tx_desc = IXGBEVF_TX_DESC(tx_ring, i);
@@ -3540,6 +3551,8 @@ static void ixgbevf_tx_map(struct ixgbevf_ring *tx_ring,
/* set next_to_watch value indicating a packet is present */
first->next_to_watch = tx_desc;
+ start = first - tx_ring->tx_buffer_info;
+ first->desc_num = (i - start >= 0) ? i - start: i + tx_ring->count -
start;
i++;
if (i == tx_ring->count)
start and i could be the same value. If you look at ixgbevf_tx_map you
should find that if the packet is contained in a single buffer then the
first and last descriptor in your send will be the same one.
