From: Jason Xing Since 'budget' parameter in ixgbe_clean_xdp_tx_irq() takes no effect, the patch removes it. No functional change here. Signed-off-by: Jason Xing --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h | 2 +- drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 6122a0abb41f..a59fd8f74b5e 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -3591,7 +3591,7 @@ int ixgbe_poll(struct napi_struct *napi, int budget) ixgbe_for_each_ring(ring, q_vector->tx) { bool wd = ring->xsk_pool ? - ixgbe_clean_xdp_tx_irq(q_vector, ring, budget) : + ixgbe_clean_xdp_tx_irq(q_vector, ring) : ixgbe_clean_tx_irq(q_vector, ring, budget); if (!wd) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h index 78deea5ec536..788722fe527a 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h @@ -42,7 +42,7 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector, const int budget); void ixgbe_xsk_clean_rx_ring(struct ixgbe_ring *rx_ring); bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector, - struct ixgbe_ring *tx_ring, int napi_budget); + struct ixgbe_ring *tx_ring); int ixgbe_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags); void ixgbe_xsk_clean_tx_ring(struct ixgbe_ring *tx_ring); diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index ac58964b2f08..0ade15058d98 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -454,7 +454,7 @@ static void ixgbe_clean_xdp_tx_buffer(struct ixgbe_ring *tx_ring, } bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector, - struct ixgbe_ring *tx_ring, int napi_budget) + struct ixgbe_ring *tx_ring) { u16 ntc = tx_ring->next_to_clean, ntu = tx_ring->next_to_use; unsigned int total_packets = 0, total_bytes = 0; -- 2.41.3 From: Jason Xing Resolve the budget underflow which leads to returning true in ixgbe_xmit_zc even when the budget of descs are thoroughly consumed. Before this patch, when the budget is decreased to zero and finishes sending the last allowed desc in ixgbe_xmit_zc, it will always turn back and enter into the while() statement to see if it should keep processing packets, but in the meantime it unexpectedly decreases the value again to 'unsigned int (0--)', namely, UINT_MAX. Finally, the ixgbe_xmit_zc returns true, showing 'we complete cleaning the budget'. That also means 'clean_complete = true' in ixgbe_poll. The true theory behind this is if that budget number of descs are consumed, it implies that we might have more descs to be done. So we should return false in ixgbe_xmit_zc to tell napi poll to find another chance to start polling to handle the rest of descs. On the contrary, returning true here means job done and we know we finish all the possible descs this time and we don't intend to start a new napi poll. It is apparently against our expectations. Please also see how ixgbe_clean_tx_irq() handles the problem: it uses do..while() statement to make sure the budget can be decreased to zero at most and the underflow never happens. Fixes: 8221c5eba8c1 ("ixgbe: add AF_XDP zero-copy Tx support") Signed-off-by: Jason Xing --- drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index 0ade15058d98..a463c5ac9c7c 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -398,7 +398,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) dma_addr_t dma; u32 cmd_type; - while (budget-- > 0) { + while (likely(budget)) { if (unlikely(!ixgbe_desc_unused(xdp_ring))) { work_done = false; break; @@ -433,6 +433,8 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) xdp_ring->next_to_use++; if (xdp_ring->next_to_use == xdp_ring->count) xdp_ring->next_to_use = 0; + + budget--; } if (tx_desc) { -- 2.41.3 From: Jason Xing - Adjust ixgbe_desc_unused as the budget value. - Avoid checking desc_unused over and over again in the loop. The patch makes ixgbe follow i40e driver that was done in commit 1fd972ebe523 ("i40e: move check of full Tx ring to outside of send loop"). [ Note that the above i40e patch has problem when ixgbe_desc_unused(tx_ring) returns zero. The zero value as the budget value means we don't have any possible descs to be sent, so it should return true instead to tell the napi poll not to launch another poll to handle tx packets. Even though that patch behaves correctly by returning true in this case, it happens because of the unexpected underflow of the budget. Taking the current version of i40e_xmit_zc() as an example, it returns true as expected. ] Hence, this patch adds a standalone if statement of zero budget in front of ixgbe_xmit_zc() as explained before. Use ixgbe_desc_unused to replace the original fixed budget with the number of available slots in the Tx ring. It can gain some performance. Signed-off-by: Jason Xing --- drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index a463c5ac9c7c..f3d3f5c1cdc7 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -393,17 +393,14 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) struct xsk_buff_pool *pool = xdp_ring->xsk_pool; union ixgbe_adv_tx_desc *tx_desc = NULL; struct ixgbe_tx_buffer *tx_bi; - bool work_done = true; struct xdp_desc desc; dma_addr_t dma; u32 cmd_type; - while (likely(budget)) { - if (unlikely(!ixgbe_desc_unused(xdp_ring))) { - work_done = false; - break; - } + if (!budget) + return true; + while (likely(budget)) { if (!netif_carrier_ok(xdp_ring->netdev)) break; @@ -442,7 +439,7 @@ static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) xsk_tx_release(pool); } - return !!budget && work_done; + return !!budget; } static void ixgbe_clean_xdp_tx_buffer(struct ixgbe_ring *tx_ring, @@ -505,7 +502,7 @@ bool ixgbe_clean_xdp_tx_irq(struct ixgbe_q_vector *q_vector, if (xsk_uses_need_wakeup(pool)) xsk_set_tx_need_wakeup(pool); - return ixgbe_xmit_zc(tx_ring, q_vector->tx.work_limit); + return ixgbe_xmit_zc(tx_ring, ixgbe_desc_unused(tx_ring)); } int ixgbe_xsk_wakeup(struct net_device *dev, u32 qid, u32 flags) -- 2.41.3 From: Jason Xing Like what i40e driver initially did in commit 3106c580fb7cf ("i40e: Use batched xsk Tx interfaces to increase performance"), use the batched xsk feature to transmit packets. Signed-off-by: Jason Xing --- drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 106 +++++++++++++------ 1 file changed, 72 insertions(+), 34 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index f3d3f5c1cdc7..9fe2c4bf8bc5 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -2,12 +2,15 @@ /* Copyright(c) 2018 Intel Corporation. */ #include +#include #include #include #include "ixgbe.h" #include "ixgbe_txrx_common.h" +#define PKTS_PER_BATCH 4 + struct xsk_buff_pool *ixgbe_xsk_pool(struct ixgbe_adapter *adapter, struct ixgbe_ring *ring) { @@ -388,58 +391,93 @@ void ixgbe_xsk_clean_rx_ring(struct ixgbe_ring *rx_ring) } } -static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) +static void ixgbe_set_rs_bit(struct ixgbe_ring *xdp_ring) +{ + u16 ntu = xdp_ring->next_to_use ? xdp_ring->next_to_use - 1 : xdp_ring->count - 1; + union ixgbe_adv_tx_desc *tx_desc; + + tx_desc = IXGBE_TX_DESC(xdp_ring, ntu); + tx_desc->read.cmd_type_len |= cpu_to_le32(IXGBE_TXD_CMD_RS); +} + +static void ixgbe_xmit_pkt(struct ixgbe_ring *xdp_ring, struct xdp_desc *desc, + int i) + { struct xsk_buff_pool *pool = xdp_ring->xsk_pool; union ixgbe_adv_tx_desc *tx_desc = NULL; struct ixgbe_tx_buffer *tx_bi; - struct xdp_desc desc; dma_addr_t dma; u32 cmd_type; - if (!budget) - return true; + dma = xsk_buff_raw_get_dma(pool, desc[i].addr); + xsk_buff_raw_dma_sync_for_device(pool, dma, desc[i].len); - while (likely(budget)) { - if (!netif_carrier_ok(xdp_ring->netdev)) - break; + tx_bi = &xdp_ring->tx_buffer_info[xdp_ring->next_to_use]; + tx_bi->bytecount = desc[i].len; + tx_bi->xdpf = NULL; + tx_bi->gso_segs = 1; - if (!xsk_tx_peek_desc(pool, &desc)) - break; + tx_desc = IXGBE_TX_DESC(xdp_ring, xdp_ring->next_to_use); + tx_desc->read.buffer_addr = cpu_to_le64(dma); - dma = xsk_buff_raw_get_dma(pool, desc.addr); - xsk_buff_raw_dma_sync_for_device(pool, dma, desc.len); + cmd_type = IXGBE_ADVTXD_DTYP_DATA | + IXGBE_ADVTXD_DCMD_DEXT | + IXGBE_ADVTXD_DCMD_IFCS; + cmd_type |= desc[i].len | IXGBE_TXD_CMD_EOP; + tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type); + tx_desc->read.olinfo_status = + cpu_to_le32(desc[i].len << IXGBE_ADVTXD_PAYLEN_SHIFT); - tx_bi = &xdp_ring->tx_buffer_info[xdp_ring->next_to_use]; - tx_bi->bytecount = desc.len; - tx_bi->xdpf = NULL; - tx_bi->gso_segs = 1; + xdp_ring->next_to_use++; +} - tx_desc = IXGBE_TX_DESC(xdp_ring, xdp_ring->next_to_use); - tx_desc->read.buffer_addr = cpu_to_le64(dma); +static void ixgbe_xmit_pkt_batch(struct ixgbe_ring *xdp_ring, struct xdp_desc *desc) +{ + u32 i; - /* put descriptor type bits */ - cmd_type = IXGBE_ADVTXD_DTYP_DATA | - IXGBE_ADVTXD_DCMD_DEXT | - IXGBE_ADVTXD_DCMD_IFCS; - cmd_type |= desc.len | IXGBE_TXD_CMD; - tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type); - tx_desc->read.olinfo_status = - cpu_to_le32(desc.len << IXGBE_ADVTXD_PAYLEN_SHIFT); + unrolled_count(PKTS_PER_BATCH) + for (i = 0; i < PKTS_PER_BATCH; i++) + ixgbe_xmit_pkt(xdp_ring, desc, i); +} - xdp_ring->next_to_use++; - if (xdp_ring->next_to_use == xdp_ring->count) - xdp_ring->next_to_use = 0; +static void ixgbe_fill_tx_hw_ring(struct ixgbe_ring *xdp_ring, + struct xdp_desc *descs, u32 nb_pkts) +{ + u32 batched, leftover, i; + + batched = nb_pkts & ~(PKTS_PER_BATCH - 1); + leftover = nb_pkts & (PKTS_PER_BATCH - 1); + for (i = 0; i < batched; i += PKTS_PER_BATCH) + ixgbe_xmit_pkt_batch(xdp_ring, &descs[i]); + for (i = batched; i < batched + leftover; i++) + ixgbe_xmit_pkt(xdp_ring, &descs[i], 0); +} - budget--; - } +static bool ixgbe_xmit_zc(struct ixgbe_ring *xdp_ring, unsigned int budget) +{ + struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs; + u32 nb_pkts, nb_processed = 0; - if (tx_desc) { - ixgbe_xdp_ring_update_tail(xdp_ring); - xsk_tx_release(pool); + if (!netif_carrier_ok(xdp_ring->netdev)) + return true; + + nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget); + if (!nb_pkts) + return true; + + if (xdp_ring->next_to_use + nb_pkts >= xdp_ring->count) { + nb_processed = xdp_ring->count - xdp_ring->next_to_use; + ixgbe_fill_tx_hw_ring(xdp_ring, descs, nb_processed); + xdp_ring->next_to_use = 0; } - return !!budget; + ixgbe_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed); + + ixgbe_set_rs_bit(xdp_ring); + ixgbe_xdp_ring_update_tail(xdp_ring); + + return nb_pkts < budget; } static void ixgbe_clean_xdp_tx_buffer(struct ixgbe_ring *tx_ring, -- 2.41.3 From: Jason Xing Use the common interface to see if the desc is the end of packets. If so, set IXGBE_TXD_CMD_EOP bit instead of setting for all preceding descriptors. This is also how i40e driver did in commit a92b96c4ae10 ("i40e: xsk: add TX multi-buffer support"). Signed-off-by: Jason Xing --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++++ drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index a59fd8f74b5e..c34737065f9e 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -52,6 +52,8 @@ #include "ixgbe_txrx_common.h" #include "devlink/devlink.h" +#define IXGBE_MAX_BUFFER_TXD 4 + char ixgbe_driver_name[] = "ixgbe"; static const char ixgbe_driver_string[] = "Intel(R) 10 Gigabit PCI Express Network Driver"; @@ -11805,6 +11807,8 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent) netdev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT | NETDEV_XDP_ACT_XSK_ZEROCOPY; + netdev->xdp_zc_max_segs = IXGBE_MAX_BUFFER_TXD; + /* MTU range: 68 - 9710 */ netdev->min_mtu = ETH_MIN_MTU; netdev->max_mtu = IXGBE_MAX_JUMBO_FRAME_SIZE - (ETH_HLEN + ETH_FCS_LEN); diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index 9fe2c4bf8bc5..3d9fa4f2403e 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -424,7 +424,9 @@ static void ixgbe_xmit_pkt(struct ixgbe_ring *xdp_ring, struct xdp_desc *desc, cmd_type = IXGBE_ADVTXD_DTYP_DATA | IXGBE_ADVTXD_DCMD_DEXT | IXGBE_ADVTXD_DCMD_IFCS; - cmd_type |= desc[i].len | IXGBE_TXD_CMD_EOP; + cmd_type |= desc[i].len; + if (xsk_is_eop_desc(&desc[i])) + cmd_type |= IXGBE_TXD_CMD_EOP; tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type); tx_desc->read.olinfo_status = cpu_to_le32(desc[i].len << IXGBE_ADVTXD_PAYLEN_SHIFT); -- 2.41.3