* [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool
@ 2024-05-29 11:23 ` Maciej Fijalkowski
0 siblings, 0 replies; 46+ messages in thread
From: Maciej Fijalkowski @ 2024-05-29 11:23 UTC (permalink / raw)
To: intel-wired-lan
Cc: netdev, anthony.l.nguyen, magnus.karlsson, michal.kubiak,
larysa.zaremba, Maciej Fijalkowski
xsk_buff_pool pointers that ice ring structs hold are updated via
ndo_bpf that is executed in process context while it can be read by
remote CPU at the same time within NAPI poll. Use synchronize_net()
after pointer update and {READ,WRITE}_ONCE() when working with mentioned
pointer.
Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
drivers/net/ethernet/intel/ice/ice.h | 6 +-
drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
6 files changed, 59 insertions(+), 39 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index da8c8afebc93..701a61d791dd 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
* Returns a pointer to xsk_buff_pool structure if there is a buffer pool
* present, NULL otherwise.
*/
-static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
+static inline void ice_xsk_pool(struct ice_rx_ring *ring)
{
struct ice_vsi *vsi = ring->vsi;
u16 qid = ring->q_index;
- return ice_get_xp_from_qid(vsi, qid);
+ WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
}
/**
@@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
if (!ring)
return;
- ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
+ WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
index 5d396c1a7731..f3dfce136106 100644
--- a/drivers/net/ethernet/intel/ice/ice_base.c
+++ b/drivers/net/ethernet/intel/ice/ice_base.c
@@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
return err;
}
- ring->xsk_pool = ice_xsk_pool(ring);
+ ice_xsk_pool(ring);
if (ring->xsk_pool) {
xdp_rxq_info_unreg(&ring->xdp_rxq);
@@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
return 0;
}
- ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
+ ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
if (!ok) {
u16 pf_q = ring->vsi->rxq_map[ring->q_index];
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 1b61ca3a6eb6..15a6805ac2a1 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
ice_for_each_rxq(vsi, i) {
struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
- if (rx_ring->xsk_pool)
+ if (READ_ONCE(rx_ring->xsk_pool))
napi_schedule(&rx_ring->q_vector->napi);
}
}
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 8bb743f78fcb..f4b2b1bca234 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
ice_for_each_tx_ring(tx_ring, q_vector->tx) {
bool wd;
- if (tx_ring->xsk_pool)
+ if (READ_ONCE(tx_ring->xsk_pool))
wd = ice_xmit_zc(tx_ring);
else if (ice_ring_is_xdp(tx_ring))
wd = true;
@@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
* comparison in the irq context instead of many inside the
* ice_clean_rx_irq function and makes the codebase cleaner.
*/
- cleaned = rx_ring->xsk_pool ?
+ cleaned = READ_ONCE(rx_ring->xsk_pool) ?
ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
ice_clean_rx_irq(rx_ring, budget_per_ring);
work_done += cleaned;
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 8c5006f37310..693f0e3a37ce 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
ice_qvec_toggle_napi(vsi, q_vector, true);
ice_qvec_ena_irq(vsi, q_vector);
+ /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
+ synchronize_net();
netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
netif_carrier_on(vsi->netdev);
clear_bit(ICE_CFG_BUSY, vsi->state);
@@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
/**
* __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
* @rx_ring: Rx ring
+ * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
* @count: The number of buffers to allocate
*
* Place the @count of descriptors onto Rx ring. Handle the ring wrap
@@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
*
* Returns true if all allocations were successful, false if any fail.
*/
-static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
+static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
+ struct xsk_buff_pool *xsk_pool, u16 count)
{
u32 nb_buffs_extra = 0, nb_buffs = 0;
union ice_32b_rx_flex_desc *rx_desc;
@@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
xdp = ice_xdp_buf(rx_ring, ntu);
if (ntu + count >= rx_ring->count) {
- nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
- rx_desc,
+ nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
rx_ring->count - ntu);
if (nb_buffs_extra != rx_ring->count - ntu) {
ntu += nb_buffs_extra;
@@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
ice_release_rx_desc(rx_ring, 0);
}
- nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
+ nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
ntu += nb_buffs;
if (ntu == rx_ring->count)
@@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
/**
* ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
* @rx_ring: Rx ring
+ * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
* @count: The number of buffers to allocate
*
* Wrapper for internal allocation routine; figure out how many tail
@@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
*
* Returns true if all calls to internal alloc routine succeeded
*/
-bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
+bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
+ struct xsk_buff_pool *xsk_pool, u16 count)
{
u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
u16 leftover, i, tail_bumps;
@@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
leftover = count - (tail_bumps * rx_thresh);
for (i = 0; i < tail_bumps; i++)
- if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
+ if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
return false;
- return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
+ return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
}
/**
@@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
if (xdp_ring->next_to_clean >= cnt)
xdp_ring->next_to_clean -= cnt;
if (xsk_frames)
- xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
+ xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
return completed_frames;
}
@@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
dma_addr_t dma;
dma = xsk_buff_xdp_get_dma(xdp);
- xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
+ xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
+ dma, size);
tx_buf->xdp = xdp;
tx_buf->type = ICE_TX_BUF_XSK_TX;
@@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
if (!err)
return ICE_XDP_REDIR;
- if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
+ if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
+ err == -ENOBUFS)
result = ICE_XDP_EXIT;
else
result = ICE_XDP_CONSUMED;
@@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
*/
int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
{
+ struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
unsigned int total_rx_bytes = 0, total_rx_packets = 0;
- struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
u32 ntc = rx_ring->next_to_clean;
u32 ntu = rx_ring->next_to_use;
struct xdp_buff *first = NULL;
@@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
rx_ring->next_to_clean = ntc;
entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
- failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
+ failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
+ entries_to_alloc);
ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
@@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
/**
* ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
* @xdp_ring: XDP ring to produce the HW Tx descriptor on
+ * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
* @desc: AF_XDP descriptor to pull the DMA address and length from
* @total_bytes: bytes accumulator that will be used for stats update
*/
-static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
+static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
+ struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
unsigned int *total_bytes)
{
struct ice_tx_desc *tx_desc;
dma_addr_t dma;
- dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
- xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
+ dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
+ xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
tx_desc->buf_addr = cpu_to_le64(dma);
@@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
/**
* ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
* @xdp_ring: XDP ring to produce the HW Tx descriptors on
+ * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
* @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
* @total_bytes: bytes accumulator that will be used for stats update
*/
-static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
+static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
+ struct xsk_buff_pool *xsk_pool,
+ struct xdp_desc *descs,
unsigned int *total_bytes)
{
u16 ntu = xdp_ring->next_to_use;
@@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
dma_addr_t dma;
- dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
- xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
+ dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
+ xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
tx_desc->buf_addr = cpu_to_le64(dma);
@@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
/**
* ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
* @xdp_ring: XDP ring to produce the HW Tx descriptors on
+ * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
* @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
* @nb_pkts: count of packets to be send
* @total_bytes: bytes accumulator that will be used for stats update
*/
-static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
- u32 nb_pkts, unsigned int *total_bytes)
+static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
+ struct xsk_buff_pool *xsk_pool,
+ struct xdp_desc *descs, u32 nb_pkts,
+ unsigned int *total_bytes)
{
u32 batched, leftover, i;
batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
leftover = nb_pkts & (PKTS_PER_BATCH - 1);
for (i = 0; i < batched; i += PKTS_PER_BATCH)
- ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
+ ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
for (; i < batched + leftover; i++)
- ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
+ ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
}
/**
@@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
*/
bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
{
- struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
+ struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
+ struct xdp_desc *descs = xsk_pool->tx_descs;
u32 nb_pkts, nb_processed = 0;
unsigned int total_bytes = 0;
int budget;
@@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
budget = ICE_DESC_UNUSED(xdp_ring);
budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
- nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
+ nb_pkts = xsk_tx_peek_release_desc_batch(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;
- ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
+ ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
+ &total_bytes);
xdp_ring->next_to_use = 0;
}
- ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
- &total_bytes);
+ ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
+ nb_pkts - nb_processed, &total_bytes);
ice_set_rs_bit(xdp_ring);
ice_xdp_ring_update_tail(xdp_ring);
ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
- if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
- xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
+ if (xsk_uses_need_wakeup(xsk_pool))
+ xsk_set_tx_need_wakeup(xsk_pool);
return nb_pkts < budget;
}
@@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
ring = vsi->rx_rings[queue_id]->xdp_ring;
- if (!ring->xsk_pool)
+ if (!READ_ONCE(ring->xsk_pool))
return -EINVAL;
/* The idea here is that if NAPI is running, mark a miss, so
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
index 6fa181f080ef..4cd2d62a0836 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.h
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
@@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
u16 qid);
int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
-bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
+bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
+ struct xsk_buff_pool *xsk_pool, u16 count);
bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
@@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
static inline bool
ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
+ struct xsk_buff_pool __always_unused *xsk_pool,
u16 __always_unused count)
{
return false;
--
2.34.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool
2024-05-29 11:23 ` [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool Maciej Fijalkowski
@ 2024-05-31 23:49 ` Nelson, Shannon
-1 siblings, 0 replies; 46+ messages in thread
From: Nelson, Shannon @ 2024-05-31 23:49 UTC (permalink / raw)
To: Maciej Fijalkowski, intel-wired-lan
Cc: netdev, michal.kubiak, anthony.l.nguyen, magnus.karlsson,
larysa.zaremba
On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
>
> xsk_buff_pool pointers that ice ring structs hold are updated via
> ndo_bpf that is executed in process context while it can be read by
> remote CPU at the same time within NAPI poll. Use synchronize_net()
> after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> pointer.
>
> Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice.h | 6 +-
> drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> 6 files changed, 59 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index da8c8afebc93..701a61d791dd 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> * present, NULL otherwise.
> */
> -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
Since this patch is changing the logic here you mighht want to change
the name of the function. Instead of getting a pointer with no side
effects it is now doing something, so a better name might be
ice_set_xsk_pool() to reflect the resulting side effect.
sln
> {
> struct ice_vsi *vsi = ring->vsi;
> u16 qid = ring->q_index;
>
> - return ice_get_xp_from_qid(vsi, qid);
> + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> }
>
> /**
> @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> if (!ring)
> return;
>
> - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 5d396c1a7731..f3dfce136106 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> return err;
> }
>
> - ring->xsk_pool = ice_xsk_pool(ring);
> + ice_xsk_pool(ring);
> if (ring->xsk_pool) {
> xdp_rxq_info_unreg(&ring->xdp_rxq);
>
> @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> return 0;
> }
>
> - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> if (!ok) {
> u16 pf_q = ring->vsi->rxq_map[ring->q_index];
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 1b61ca3a6eb6..15a6805ac2a1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> ice_for_each_rxq(vsi, i) {
> struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
>
> - if (rx_ring->xsk_pool)
> + if (READ_ONCE(rx_ring->xsk_pool))
> napi_schedule(&rx_ring->q_vector->napi);
> }
> }
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 8bb743f78fcb..f4b2b1bca234 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> bool wd;
>
> - if (tx_ring->xsk_pool)
> + if (READ_ONCE(tx_ring->xsk_pool))
> wd = ice_xmit_zc(tx_ring);
> else if (ice_ring_is_xdp(tx_ring))
> wd = true;
> @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> * comparison in the irq context instead of many inside the
> * ice_clean_rx_irq function and makes the codebase cleaner.
> */
> - cleaned = rx_ring->xsk_pool ?
> + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> ice_clean_rx_irq(rx_ring, budget_per_ring);
> work_done += cleaned;
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 8c5006f37310..693f0e3a37ce 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> ice_qvec_toggle_napi(vsi, q_vector, true);
> ice_qvec_ena_irq(vsi, q_vector);
>
> + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> + synchronize_net();
> netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> netif_carrier_on(vsi->netdev);
> clear_bit(ICE_CFG_BUSY, vsi->state);
> @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> /**
> * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> * @rx_ring: Rx ring
> + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> * @count: The number of buffers to allocate
> *
> * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> *
> * Returns true if all allocations were successful, false if any fail.
> */
> -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count)
> {
> u32 nb_buffs_extra = 0, nb_buffs = 0;
> union ice_32b_rx_flex_desc *rx_desc;
> @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> xdp = ice_xdp_buf(rx_ring, ntu);
>
> if (ntu + count >= rx_ring->count) {
> - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> - rx_desc,
> + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> rx_ring->count - ntu);
> if (nb_buffs_extra != rx_ring->count - ntu) {
> ntu += nb_buffs_extra;
> @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> ice_release_rx_desc(rx_ring, 0);
> }
>
> - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
>
> ntu += nb_buffs;
> if (ntu == rx_ring->count)
> @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> /**
> * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> * @rx_ring: Rx ring
> + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> * @count: The number of buffers to allocate
> *
> * Wrapper for internal allocation routine; figure out how many tail
> @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> *
> * Returns true if all calls to internal alloc routine succeeded
> */
> -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count)
> {
> u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> u16 leftover, i, tail_bumps;
> @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> leftover = count - (tail_bumps * rx_thresh);
>
> for (i = 0; i < tail_bumps; i++)
> - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> return false;
> - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> }
>
> /**
> @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> if (xdp_ring->next_to_clean >= cnt)
> xdp_ring->next_to_clean -= cnt;
> if (xsk_frames)
> - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
>
> return completed_frames;
> }
> @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> dma_addr_t dma;
>
> dma = xsk_buff_xdp_get_dma(xdp);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> + dma, size);
>
> tx_buf->xdp = xdp;
> tx_buf->type = ICE_TX_BUF_XSK_TX;
> @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> if (!err)
> return ICE_XDP_REDIR;
> - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> + err == -ENOBUFS)
> result = ICE_XDP_EXIT;
> else
> result = ICE_XDP_CONSUMED;
> @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> */
> int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> {
> + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> u32 ntc = rx_ring->next_to_clean;
> u32 ntu = rx_ring->next_to_use;
> struct xdp_buff *first = NULL;
> @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> rx_ring->next_to_clean = ntc;
> entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> + entries_to_alloc);
>
> ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> /**
> * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @desc: AF_XDP descriptor to pull the DMA address and length from
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> unsigned int *total_bytes)
> {
> struct ice_tx_desc *tx_desc;
> dma_addr_t dma;
>
> - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
>
> tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> tx_desc->buf_addr = cpu_to_le64(dma);
> @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> /**
> * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool,
> + struct xdp_desc *descs,
> unsigned int *total_bytes)
> {
> u16 ntu = xdp_ring->next_to_use;
> @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> dma_addr_t dma;
>
> - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
>
> tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> tx_desc->buf_addr = cpu_to_le64(dma);
> @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> /**
> * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> * @nb_pkts: count of packets to be send
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> - u32 nb_pkts, unsigned int *total_bytes)
> +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool,
> + struct xdp_desc *descs, u32 nb_pkts,
> + unsigned int *total_bytes)
> {
> u32 batched, leftover, i;
>
> batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> for (i = 0; i < batched; i += PKTS_PER_BATCH)
> - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> for (; i < batched + leftover; i++)
> - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> }
>
> /**
> @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> */
> bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> {
> - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> + struct xdp_desc *descs = xsk_pool->tx_descs;
> u32 nb_pkts, nb_processed = 0;
> unsigned int total_bytes = 0;
> int budget;
> @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> budget = ICE_DESC_UNUSED(xdp_ring);
> budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
>
> - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> + &total_bytes);
> xdp_ring->next_to_use = 0;
> }
>
> - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> - &total_bytes);
> + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> + nb_pkts - nb_processed, &total_bytes);
>
> ice_set_rs_bit(xdp_ring);
> ice_xdp_ring_update_tail(xdp_ring);
> ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
>
> - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> + if (xsk_uses_need_wakeup(xsk_pool))
> + xsk_set_tx_need_wakeup(xsk_pool);
>
> return nb_pkts < budget;
> }
> @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
>
> ring = vsi->rx_rings[queue_id]->xdp_ring;
>
> - if (!ring->xsk_pool)
> + if (!READ_ONCE(ring->xsk_pool))
> return -EINVAL;
>
> /* The idea here is that if NAPI is running, mark a miss, so
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> index 6fa181f080ef..4cd2d62a0836 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> u16 qid);
> int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count);
> bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
>
> static inline bool
> ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> + struct xsk_buff_pool __always_unused *xsk_pool,
> u16 __always_unused count)
> {
> return false;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool
@ 2024-05-31 23:49 ` Nelson, Shannon
0 siblings, 0 replies; 46+ messages in thread
From: Nelson, Shannon @ 2024-05-31 23:49 UTC (permalink / raw)
To: Maciej Fijalkowski, intel-wired-lan
Cc: netdev, anthony.l.nguyen, magnus.karlsson, michal.kubiak,
larysa.zaremba
On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
>
> xsk_buff_pool pointers that ice ring structs hold are updated via
> ndo_bpf that is executed in process context while it can be read by
> remote CPU at the same time within NAPI poll. Use synchronize_net()
> after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> pointer.
>
> Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice.h | 6 +-
> drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> 6 files changed, 59 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index da8c8afebc93..701a61d791dd 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> * present, NULL otherwise.
> */
> -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
Since this patch is changing the logic here you mighht want to change
the name of the function. Instead of getting a pointer with no side
effects it is now doing something, so a better name might be
ice_set_xsk_pool() to reflect the resulting side effect.
sln
> {
> struct ice_vsi *vsi = ring->vsi;
> u16 qid = ring->q_index;
>
> - return ice_get_xp_from_qid(vsi, qid);
> + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> }
>
> /**
> @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> if (!ring)
> return;
>
> - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> index 5d396c1a7731..f3dfce136106 100644
> --- a/drivers/net/ethernet/intel/ice/ice_base.c
> +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> return err;
> }
>
> - ring->xsk_pool = ice_xsk_pool(ring);
> + ice_xsk_pool(ring);
> if (ring->xsk_pool) {
> xdp_rxq_info_unreg(&ring->xdp_rxq);
>
> @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> return 0;
> }
>
> - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> if (!ok) {
> u16 pf_q = ring->vsi->rxq_map[ring->q_index];
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 1b61ca3a6eb6..15a6805ac2a1 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> ice_for_each_rxq(vsi, i) {
> struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
>
> - if (rx_ring->xsk_pool)
> + if (READ_ONCE(rx_ring->xsk_pool))
> napi_schedule(&rx_ring->q_vector->napi);
> }
> }
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 8bb743f78fcb..f4b2b1bca234 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> bool wd;
>
> - if (tx_ring->xsk_pool)
> + if (READ_ONCE(tx_ring->xsk_pool))
> wd = ice_xmit_zc(tx_ring);
> else if (ice_ring_is_xdp(tx_ring))
> wd = true;
> @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> * comparison in the irq context instead of many inside the
> * ice_clean_rx_irq function and makes the codebase cleaner.
> */
> - cleaned = rx_ring->xsk_pool ?
> + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> ice_clean_rx_irq(rx_ring, budget_per_ring);
> work_done += cleaned;
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 8c5006f37310..693f0e3a37ce 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> ice_qvec_toggle_napi(vsi, q_vector, true);
> ice_qvec_ena_irq(vsi, q_vector);
>
> + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> + synchronize_net();
> netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> netif_carrier_on(vsi->netdev);
> clear_bit(ICE_CFG_BUSY, vsi->state);
> @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> /**
> * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> * @rx_ring: Rx ring
> + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> * @count: The number of buffers to allocate
> *
> * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> *
> * Returns true if all allocations were successful, false if any fail.
> */
> -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count)
> {
> u32 nb_buffs_extra = 0, nb_buffs = 0;
> union ice_32b_rx_flex_desc *rx_desc;
> @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> xdp = ice_xdp_buf(rx_ring, ntu);
>
> if (ntu + count >= rx_ring->count) {
> - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> - rx_desc,
> + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> rx_ring->count - ntu);
> if (nb_buffs_extra != rx_ring->count - ntu) {
> ntu += nb_buffs_extra;
> @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> ice_release_rx_desc(rx_ring, 0);
> }
>
> - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
>
> ntu += nb_buffs;
> if (ntu == rx_ring->count)
> @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> /**
> * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> * @rx_ring: Rx ring
> + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> * @count: The number of buffers to allocate
> *
> * Wrapper for internal allocation routine; figure out how many tail
> @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> *
> * Returns true if all calls to internal alloc routine succeeded
> */
> -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count)
> {
> u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> u16 leftover, i, tail_bumps;
> @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> leftover = count - (tail_bumps * rx_thresh);
>
> for (i = 0; i < tail_bumps; i++)
> - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> return false;
> - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> }
>
> /**
> @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> if (xdp_ring->next_to_clean >= cnt)
> xdp_ring->next_to_clean -= cnt;
> if (xsk_frames)
> - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
>
> return completed_frames;
> }
> @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> dma_addr_t dma;
>
> dma = xsk_buff_xdp_get_dma(xdp);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> + dma, size);
>
> tx_buf->xdp = xdp;
> tx_buf->type = ICE_TX_BUF_XSK_TX;
> @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> if (!err)
> return ICE_XDP_REDIR;
> - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> + err == -ENOBUFS)
> result = ICE_XDP_EXIT;
> else
> result = ICE_XDP_CONSUMED;
> @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> */
> int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> {
> + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> u32 ntc = rx_ring->next_to_clean;
> u32 ntu = rx_ring->next_to_use;
> struct xdp_buff *first = NULL;
> @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> rx_ring->next_to_clean = ntc;
> entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> + entries_to_alloc);
>
> ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> /**
> * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @desc: AF_XDP descriptor to pull the DMA address and length from
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> unsigned int *total_bytes)
> {
> struct ice_tx_desc *tx_desc;
> dma_addr_t dma;
>
> - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
>
> tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> tx_desc->buf_addr = cpu_to_le64(dma);
> @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> /**
> * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool,
> + struct xdp_desc *descs,
> unsigned int *total_bytes)
> {
> u16 ntu = xdp_ring->next_to_use;
> @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> dma_addr_t dma;
>
> - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
>
> tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> tx_desc->buf_addr = cpu_to_le64(dma);
> @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> /**
> * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> * @nb_pkts: count of packets to be send
> * @total_bytes: bytes accumulator that will be used for stats update
> */
> -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> - u32 nb_pkts, unsigned int *total_bytes)
> +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> + struct xsk_buff_pool *xsk_pool,
> + struct xdp_desc *descs, u32 nb_pkts,
> + unsigned int *total_bytes)
> {
> u32 batched, leftover, i;
>
> batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> for (i = 0; i < batched; i += PKTS_PER_BATCH)
> - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> for (; i < batched + leftover; i++)
> - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> }
>
> /**
> @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> */
> bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> {
> - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> + struct xdp_desc *descs = xsk_pool->tx_descs;
> u32 nb_pkts, nb_processed = 0;
> unsigned int total_bytes = 0;
> int budget;
> @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> budget = ICE_DESC_UNUSED(xdp_ring);
> budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
>
> - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> + &total_bytes);
> xdp_ring->next_to_use = 0;
> }
>
> - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> - &total_bytes);
> + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> + nb_pkts - nb_processed, &total_bytes);
>
> ice_set_rs_bit(xdp_ring);
> ice_xdp_ring_update_tail(xdp_ring);
> ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
>
> - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> + if (xsk_uses_need_wakeup(xsk_pool))
> + xsk_set_tx_need_wakeup(xsk_pool);
>
> return nb_pkts < budget;
> }
> @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
>
> ring = vsi->rx_rings[queue_id]->xdp_ring;
>
> - if (!ring->xsk_pool)
> + if (!READ_ONCE(ring->xsk_pool))
> return -EINVAL;
>
> /* The idea here is that if NAPI is running, mark a miss, so
> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> index 6fa181f080ef..4cd2d62a0836 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> u16 qid);
> int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> + struct xsk_buff_pool *xsk_pool, u16 count);
> bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
>
> static inline bool
> ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> + struct xsk_buff_pool __always_unused *xsk_pool,
> u16 __always_unused count)
> {
> return false;
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool
2024-05-31 23:49 ` [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool Nelson, Shannon
@ 2024-06-04 10:52 ` Maciej Fijalkowski
-1 siblings, 0 replies; 46+ messages in thread
From: Maciej Fijalkowski @ 2024-06-04 10:52 UTC (permalink / raw)
To: Nelson, Shannon
Cc: larysa.zaremba, netdev, michal.kubiak, anthony.l.nguyen,
intel-wired-lan, magnus.karlsson
On Fri, May 31, 2024 at 04:49:05PM -0700, Nelson, Shannon wrote:
> On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
> >
> > xsk_buff_pool pointers that ice ring structs hold are updated via
> > ndo_bpf that is executed in process context while it can be read by
> > remote CPU at the same time within NAPI poll. Use synchronize_net()
> > after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> > pointer.
> >
> > Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice.h | 6 +-
> > drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> > drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> > drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> > drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> > 6 files changed, 59 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > index da8c8afebc93..701a61d791dd 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> > * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> > * present, NULL otherwise.
> > */
> > -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> > +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
>
> Since this patch is changing the logic here you mighht want to change the
> name of the function. Instead of getting a pointer with no side effects it
> is now doing something, so a better name might be ice_set_xsk_pool() to
> reflect the resulting side effect.
Makes sense, plus the function description needs some adjustment. Will fix
on v3.
>
> sln
>
> > {
> > struct ice_vsi *vsi = ring->vsi;
> > u16 qid = ring->q_index;
> >
> > - return ice_get_xp_from_qid(vsi, qid);
> > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > }
> >
> > /**
> > @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> > if (!ring)
> > return;
> >
> > - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > }
> >
> > /**
> > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > index 5d396c1a7731..f3dfce136106 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > return err;
> > }
> >
> > - ring->xsk_pool = ice_xsk_pool(ring);
> > + ice_xsk_pool(ring);
> > if (ring->xsk_pool) {
> > xdp_rxq_info_unreg(&ring->xdp_rxq);
> >
> > @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > return 0;
> > }
> >
> > - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> > + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> > if (!ok) {
> > u16 pf_q = ring->vsi->rxq_map[ring->q_index];
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 1b61ca3a6eb6..15a6805ac2a1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> > ice_for_each_rxq(vsi, i) {
> > struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
> >
> > - if (rx_ring->xsk_pool)
> > + if (READ_ONCE(rx_ring->xsk_pool))
> > napi_schedule(&rx_ring->q_vector->napi);
> > }
> > }
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index 8bb743f78fcb..f4b2b1bca234 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> > bool wd;
> >
> > - if (tx_ring->xsk_pool)
> > + if (READ_ONCE(tx_ring->xsk_pool))
> > wd = ice_xmit_zc(tx_ring);
> > else if (ice_ring_is_xdp(tx_ring))
> > wd = true;
> > @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > * comparison in the irq context instead of many inside the
> > * ice_clean_rx_irq function and makes the codebase cleaner.
> > */
> > - cleaned = rx_ring->xsk_pool ?
> > + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> > ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> > ice_clean_rx_irq(rx_ring, budget_per_ring);
> > work_done += cleaned;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 8c5006f37310..693f0e3a37ce 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> > ice_qvec_toggle_napi(vsi, q_vector, true);
> > ice_qvec_ena_irq(vsi, q_vector);
> >
> > + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> > + synchronize_net();
> > netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> > netif_carrier_on(vsi->netdev);
> > clear_bit(ICE_CFG_BUSY, vsi->state);
> > @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > /**
> > * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > * @rx_ring: Rx ring
> > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > * @count: The number of buffers to allocate
> > *
> > * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> > @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > *
> > * Returns true if all allocations were successful, false if any fail.
> > */
> > -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count)
> > {
> > u32 nb_buffs_extra = 0, nb_buffs = 0;
> > union ice_32b_rx_flex_desc *rx_desc;
> > @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > xdp = ice_xdp_buf(rx_ring, ntu);
> >
> > if (ntu + count >= rx_ring->count) {
> > - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > - rx_desc,
> > + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> > rx_ring->count - ntu);
> > if (nb_buffs_extra != rx_ring->count - ntu) {
> > ntu += nb_buffs_extra;
> > @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > ice_release_rx_desc(rx_ring, 0);
> > }
> >
> > - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
> >
> > ntu += nb_buffs;
> > if (ntu == rx_ring->count)
> > @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > /**
> > * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > * @rx_ring: Rx ring
> > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > * @count: The number of buffers to allocate
> > *
> > * Wrapper for internal allocation routine; figure out how many tail
> > @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > *
> > * Returns true if all calls to internal alloc routine succeeded
> > */
> > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count)
> > {
> > u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> > u16 leftover, i, tail_bumps;
> > @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > leftover = count - (tail_bumps * rx_thresh);
> >
> > for (i = 0; i < tail_bumps; i++)
> > - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> > + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> > return false;
> > - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> > + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> > }
> >
> > /**
> > @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> > if (xdp_ring->next_to_clean >= cnt)
> > xdp_ring->next_to_clean -= cnt;
> > if (xsk_frames)
> > - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> > + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
> >
> > return completed_frames;
> > }
> > @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> > dma_addr_t dma;
> >
> > dma = xsk_buff_xdp_get_dma(xdp);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> > + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> > + dma, size);
> >
> > tx_buf->xdp = xdp;
> > tx_buf->type = ICE_TX_BUF_XSK_TX;
> > @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> > if (!err)
> > return ICE_XDP_REDIR;
> > - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> > + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> > + err == -ENOBUFS)
> > result = ICE_XDP_EXIT;
> > else
> > result = ICE_XDP_CONSUMED;
> > @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> > */
> > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > {
> > + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> > unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> > - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> > u32 ntc = rx_ring->next_to_clean;
> > u32 ntu = rx_ring->next_to_use;
> > struct xdp_buff *first = NULL;
> > @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > rx_ring->next_to_clean = ntc;
> > entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> > if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> > - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> > + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> > + entries_to_alloc);
> >
> > ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> > ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> > @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > /**
> > * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> > * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @desc: AF_XDP descriptor to pull the DMA address and length from
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> > unsigned int *total_bytes)
> > {
> > struct ice_tx_desc *tx_desc;
> > dma_addr_t dma;
> >
> > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> > + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
> >
> > tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> > tx_desc->buf_addr = cpu_to_le64(dma);
> > @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > /**
> > * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool,
> > + struct xdp_desc *descs,
> > unsigned int *total_bytes)
> > {
> > u16 ntu = xdp_ring->next_to_use;
> > @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> > dma_addr_t dma;
> >
> > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> > + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
> >
> > tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> > tx_desc->buf_addr = cpu_to_le64(dma);
> > @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > /**
> > * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > * @nb_pkts: count of packets to be send
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > - u32 nb_pkts, unsigned int *total_bytes)
> > +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool,
> > + struct xdp_desc *descs, u32 nb_pkts,
> > + unsigned int *total_bytes)
> > {
> > u32 batched, leftover, i;
> >
> > batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> > leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> > for (i = 0; i < batched; i += PKTS_PER_BATCH)
> > - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> > + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > for (; i < batched + leftover; i++)
> > - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> > + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > }
> >
> > /**
> > @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> > */
> > bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > {
> > - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> > + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> > + struct xdp_desc *descs = xsk_pool->tx_descs;
> > u32 nb_pkts, nb_processed = 0;
> > unsigned int total_bytes = 0;
> > int budget;
> > @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > budget = ICE_DESC_UNUSED(xdp_ring);
> > budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
> >
> > - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> > + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> > - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> > + &total_bytes);
> > xdp_ring->next_to_use = 0;
> > }
> >
> > - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> > - &total_bytes);
> > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> > + nb_pkts - nb_processed, &total_bytes);
> >
> > ice_set_rs_bit(xdp_ring);
> > ice_xdp_ring_update_tail(xdp_ring);
> > ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
> >
> > - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> > - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> > + if (xsk_uses_need_wakeup(xsk_pool))
> > + xsk_set_tx_need_wakeup(xsk_pool);
> >
> > return nb_pkts < budget;
> > }
> > @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
> >
> > ring = vsi->rx_rings[queue_id]->xdp_ring;
> >
> > - if (!ring->xsk_pool)
> > + if (!READ_ONCE(ring->xsk_pool))
> > return -EINVAL;
> >
> > /* The idea here is that if NAPI is running, mark a miss, so
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > index 6fa181f080ef..4cd2d62a0836 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> > u16 qid);
> > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> > int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count);
> > bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> > void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> > void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> > @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
> >
> > static inline bool
> > ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> > + struct xsk_buff_pool __always_unused *xsk_pool,
> > u16 __always_unused count)
> > {
> > return false;
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool
@ 2024-06-04 10:52 ` Maciej Fijalkowski
0 siblings, 0 replies; 46+ messages in thread
From: Maciej Fijalkowski @ 2024-06-04 10:52 UTC (permalink / raw)
To: Nelson, Shannon
Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson,
michal.kubiak, larysa.zaremba
On Fri, May 31, 2024 at 04:49:05PM -0700, Nelson, Shannon wrote:
> On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
> >
> > xsk_buff_pool pointers that ice ring structs hold are updated via
> > ndo_bpf that is executed in process context while it can be read by
> > remote CPU at the same time within NAPI poll. Use synchronize_net()
> > after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> > pointer.
> >
> > Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice.h | 6 +-
> > drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> > drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> > drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> > drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> > drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> > 6 files changed, 59 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > index da8c8afebc93..701a61d791dd 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> > * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> > * present, NULL otherwise.
> > */
> > -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> > +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
>
> Since this patch is changing the logic here you mighht want to change the
> name of the function. Instead of getting a pointer with no side effects it
> is now doing something, so a better name might be ice_set_xsk_pool() to
> reflect the resulting side effect.
Makes sense, plus the function description needs some adjustment. Will fix
on v3.
>
> sln
>
> > {
> > struct ice_vsi *vsi = ring->vsi;
> > u16 qid = ring->q_index;
> >
> > - return ice_get_xp_from_qid(vsi, qid);
> > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > }
> >
> > /**
> > @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> > if (!ring)
> > return;
> >
> > - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > }
> >
> > /**
> > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > index 5d396c1a7731..f3dfce136106 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > return err;
> > }
> >
> > - ring->xsk_pool = ice_xsk_pool(ring);
> > + ice_xsk_pool(ring);
> > if (ring->xsk_pool) {
> > xdp_rxq_info_unreg(&ring->xdp_rxq);
> >
> > @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > return 0;
> > }
> >
> > - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> > + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> > if (!ok) {
> > u16 pf_q = ring->vsi->rxq_map[ring->q_index];
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 1b61ca3a6eb6..15a6805ac2a1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> > ice_for_each_rxq(vsi, i) {
> > struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
> >
> > - if (rx_ring->xsk_pool)
> > + if (READ_ONCE(rx_ring->xsk_pool))
> > napi_schedule(&rx_ring->q_vector->napi);
> > }
> > }
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index 8bb743f78fcb..f4b2b1bca234 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> > bool wd;
> >
> > - if (tx_ring->xsk_pool)
> > + if (READ_ONCE(tx_ring->xsk_pool))
> > wd = ice_xmit_zc(tx_ring);
> > else if (ice_ring_is_xdp(tx_ring))
> > wd = true;
> > @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > * comparison in the irq context instead of many inside the
> > * ice_clean_rx_irq function and makes the codebase cleaner.
> > */
> > - cleaned = rx_ring->xsk_pool ?
> > + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> > ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> > ice_clean_rx_irq(rx_ring, budget_per_ring);
> > work_done += cleaned;
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 8c5006f37310..693f0e3a37ce 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> > ice_qvec_toggle_napi(vsi, q_vector, true);
> > ice_qvec_ena_irq(vsi, q_vector);
> >
> > + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> > + synchronize_net();
> > netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> > netif_carrier_on(vsi->netdev);
> > clear_bit(ICE_CFG_BUSY, vsi->state);
> > @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > /**
> > * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > * @rx_ring: Rx ring
> > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > * @count: The number of buffers to allocate
> > *
> > * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> > @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > *
> > * Returns true if all allocations were successful, false if any fail.
> > */
> > -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count)
> > {
> > u32 nb_buffs_extra = 0, nb_buffs = 0;
> > union ice_32b_rx_flex_desc *rx_desc;
> > @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > xdp = ice_xdp_buf(rx_ring, ntu);
> >
> > if (ntu + count >= rx_ring->count) {
> > - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > - rx_desc,
> > + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> > rx_ring->count - ntu);
> > if (nb_buffs_extra != rx_ring->count - ntu) {
> > ntu += nb_buffs_extra;
> > @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > ice_release_rx_desc(rx_ring, 0);
> > }
> >
> > - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
> >
> > ntu += nb_buffs;
> > if (ntu == rx_ring->count)
> > @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > /**
> > * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > * @rx_ring: Rx ring
> > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > * @count: The number of buffers to allocate
> > *
> > * Wrapper for internal allocation routine; figure out how many tail
> > @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > *
> > * Returns true if all calls to internal alloc routine succeeded
> > */
> > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count)
> > {
> > u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> > u16 leftover, i, tail_bumps;
> > @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > leftover = count - (tail_bumps * rx_thresh);
> >
> > for (i = 0; i < tail_bumps; i++)
> > - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> > + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> > return false;
> > - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> > + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> > }
> >
> > /**
> > @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> > if (xdp_ring->next_to_clean >= cnt)
> > xdp_ring->next_to_clean -= cnt;
> > if (xsk_frames)
> > - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> > + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
> >
> > return completed_frames;
> > }
> > @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> > dma_addr_t dma;
> >
> > dma = xsk_buff_xdp_get_dma(xdp);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> > + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> > + dma, size);
> >
> > tx_buf->xdp = xdp;
> > tx_buf->type = ICE_TX_BUF_XSK_TX;
> > @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> > if (!err)
> > return ICE_XDP_REDIR;
> > - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> > + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> > + err == -ENOBUFS)
> > result = ICE_XDP_EXIT;
> > else
> > result = ICE_XDP_CONSUMED;
> > @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> > */
> > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > {
> > + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> > unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> > - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> > u32 ntc = rx_ring->next_to_clean;
> > u32 ntu = rx_ring->next_to_use;
> > struct xdp_buff *first = NULL;
> > @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > rx_ring->next_to_clean = ntc;
> > entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> > if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> > - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> > + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> > + entries_to_alloc);
> >
> > ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> > ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> > @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > /**
> > * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> > * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @desc: AF_XDP descriptor to pull the DMA address and length from
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> > unsigned int *total_bytes)
> > {
> > struct ice_tx_desc *tx_desc;
> > dma_addr_t dma;
> >
> > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> > + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
> >
> > tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> > tx_desc->buf_addr = cpu_to_le64(dma);
> > @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > /**
> > * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool,
> > + struct xdp_desc *descs,
> > unsigned int *total_bytes)
> > {
> > u16 ntu = xdp_ring->next_to_use;
> > @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> > dma_addr_t dma;
> >
> > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> > + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
> >
> > tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> > tx_desc->buf_addr = cpu_to_le64(dma);
> > @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > /**
> > * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > * @nb_pkts: count of packets to be send
> > * @total_bytes: bytes accumulator that will be used for stats update
> > */
> > -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > - u32 nb_pkts, unsigned int *total_bytes)
> > +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> > + struct xsk_buff_pool *xsk_pool,
> > + struct xdp_desc *descs, u32 nb_pkts,
> > + unsigned int *total_bytes)
> > {
> > u32 batched, leftover, i;
> >
> > batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> > leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> > for (i = 0; i < batched; i += PKTS_PER_BATCH)
> > - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> > + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > for (; i < batched + leftover; i++)
> > - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> > + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > }
> >
> > /**
> > @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> > */
> > bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > {
> > - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> > + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> > + struct xdp_desc *descs = xsk_pool->tx_descs;
> > u32 nb_pkts, nb_processed = 0;
> > unsigned int total_bytes = 0;
> > int budget;
> > @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > budget = ICE_DESC_UNUSED(xdp_ring);
> > budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
> >
> > - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> > + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> > - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> > + &total_bytes);
> > xdp_ring->next_to_use = 0;
> > }
> >
> > - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> > - &total_bytes);
> > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> > + nb_pkts - nb_processed, &total_bytes);
> >
> > ice_set_rs_bit(xdp_ring);
> > ice_xdp_ring_update_tail(xdp_ring);
> > ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
> >
> > - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> > - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> > + if (xsk_uses_need_wakeup(xsk_pool))
> > + xsk_set_tx_need_wakeup(xsk_pool);
> >
> > return nb_pkts < budget;
> > }
> > @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
> >
> > ring = vsi->rx_rings[queue_id]->xdp_ring;
> >
> > - if (!ring->xsk_pool)
> > + if (!READ_ONCE(ring->xsk_pool))
> > return -EINVAL;
> >
> > /* The idea here is that if NAPI is running, mark a miss, so
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > index 6fa181f080ef..4cd2d62a0836 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> > u16 qid);
> > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> > int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > + struct xsk_buff_pool *xsk_pool, u16 count);
> > bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> > void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> > void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> > @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
> >
> > static inline bool
> > ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> > + struct xsk_buff_pool __always_unused *xsk_pool,
> > u16 __always_unused count)
> > {
> > return false;
> > --
> > 2.34.1
> >
> >
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool
2024-06-04 10:52 ` [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool Maciej Fijalkowski
@ 2024-06-04 11:12 ` Maciej Fijalkowski
-1 siblings, 0 replies; 46+ messages in thread
From: Maciej Fijalkowski @ 2024-06-04 11:12 UTC (permalink / raw)
To: Nelson, Shannon
Cc: larysa.zaremba, netdev, michal.kubiak, anthony.l.nguyen,
intel-wired-lan, magnus.karlsson
On Tue, Jun 04, 2024 at 12:52:16PM +0200, Maciej Fijalkowski wrote:
> On Fri, May 31, 2024 at 04:49:05PM -0700, Nelson, Shannon wrote:
> > On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
> > >
> > > xsk_buff_pool pointers that ice ring structs hold are updated via
> > > ndo_bpf that is executed in process context while it can be read by
> > > remote CPU at the same time within NAPI poll. Use synchronize_net()
> > > after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> > > pointer.
> > >
> > > Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/ice/ice.h | 6 +-
> > > drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> > > drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> > > drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> > > drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> > > drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> > > 6 files changed, 59 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > > index da8c8afebc93..701a61d791dd 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> > > * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> > > * present, NULL otherwise.
> > > */
> > > -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> > > +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
> >
> > Since this patch is changing the logic here you mighht want to change the
> > name of the function. Instead of getting a pointer with no side effects it
> > is now doing something, so a better name might be ice_set_xsk_pool() to
> > reflect the resulting side effect.
>
> Makes sense, plus the function description needs some adjustment. Will fix
> on v3.
Having second thought on this, I'll go only with
s/ice_xsk_pool/ice_rx_xsk_pool/ to align with Tx side. Tx side also lacks
the 'set' keyword in its naming, but I don't want to touch this here.
I currently don't see a reason for these functions being inlined as we are
not setting xsk pool pointers in the hot path at all, therefore I can go
later on with a patch dedicated for -next that fixes naming and drops the
inlining.
>
> >
> > sln
> >
> > > {
> > > struct ice_vsi *vsi = ring->vsi;
> > > u16 qid = ring->q_index;
> > >
> > > - return ice_get_xp_from_qid(vsi, qid);
> > > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > > }
> > >
> > > /**
> > > @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> > > if (!ring)
> > > return;
> > >
> > > - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> > > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > > }
> > >
> > > /**
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > > index 5d396c1a7731..f3dfce136106 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > > @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > > return err;
> > > }
> > >
> > > - ring->xsk_pool = ice_xsk_pool(ring);
> > > + ice_xsk_pool(ring);
> > > if (ring->xsk_pool) {
> > > xdp_rxq_info_unreg(&ring->xdp_rxq);
> > >
> > > @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > > return 0;
> > > }
> > >
> > > - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> > > + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> > > if (!ok) {
> > > u16 pf_q = ring->vsi->rxq_map[ring->q_index];
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index 1b61ca3a6eb6..15a6805ac2a1 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> > > ice_for_each_rxq(vsi, i) {
> > > struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
> > >
> > > - if (rx_ring->xsk_pool)
> > > + if (READ_ONCE(rx_ring->xsk_pool))
> > > napi_schedule(&rx_ring->q_vector->napi);
> > > }
> > > }
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > index 8bb743f78fcb..f4b2b1bca234 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > > ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> > > bool wd;
> > >
> > > - if (tx_ring->xsk_pool)
> > > + if (READ_ONCE(tx_ring->xsk_pool))
> > > wd = ice_xmit_zc(tx_ring);
> > > else if (ice_ring_is_xdp(tx_ring))
> > > wd = true;
> > > @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > > * comparison in the irq context instead of many inside the
> > > * ice_clean_rx_irq function and makes the codebase cleaner.
> > > */
> > > - cleaned = rx_ring->xsk_pool ?
> > > + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> > > ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> > > ice_clean_rx_irq(rx_ring, budget_per_ring);
> > > work_done += cleaned;
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > index 8c5006f37310..693f0e3a37ce 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> > > ice_qvec_toggle_napi(vsi, q_vector, true);
> > > ice_qvec_ena_irq(vsi, q_vector);
> > >
> > > + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> > > + synchronize_net();
> > > netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> > > netif_carrier_on(vsi->netdev);
> > > clear_bit(ICE_CFG_BUSY, vsi->state);
> > > @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > > /**
> > > * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > > * @rx_ring: Rx ring
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > > * @count: The number of buffers to allocate
> > > *
> > > * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> > > @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > > *
> > > * Returns true if all allocations were successful, false if any fail.
> > > */
> > > -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count)
> > > {
> > > u32 nb_buffs_extra = 0, nb_buffs = 0;
> > > union ice_32b_rx_flex_desc *rx_desc;
> > > @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > xdp = ice_xdp_buf(rx_ring, ntu);
> > >
> > > if (ntu + count >= rx_ring->count) {
> > > - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > > - rx_desc,
> > > + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> > > rx_ring->count - ntu);
> > > if (nb_buffs_extra != rx_ring->count - ntu) {
> > > ntu += nb_buffs_extra;
> > > @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > ice_release_rx_desc(rx_ring, 0);
> > > }
> > >
> > > - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > > + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
> > >
> > > ntu += nb_buffs;
> > > if (ntu == rx_ring->count)
> > > @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > /**
> > > * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > > * @rx_ring: Rx ring
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > > * @count: The number of buffers to allocate
> > > *
> > > * Wrapper for internal allocation routine; figure out how many tail
> > > @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > *
> > > * Returns true if all calls to internal alloc routine succeeded
> > > */
> > > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count)
> > > {
> > > u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> > > u16 leftover, i, tail_bumps;
> > > @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > leftover = count - (tail_bumps * rx_thresh);
> > >
> > > for (i = 0; i < tail_bumps; i++)
> > > - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> > > + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> > > return false;
> > > - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> > > + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> > > }
> > >
> > > /**
> > > @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> > > if (xdp_ring->next_to_clean >= cnt)
> > > xdp_ring->next_to_clean -= cnt;
> > > if (xsk_frames)
> > > - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> > > + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
> > >
> > > return completed_frames;
> > > }
> > > @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> > > dma_addr_t dma;
> > >
> > > dma = xsk_buff_xdp_get_dma(xdp);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> > > + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> > > + dma, size);
> > >
> > > tx_buf->xdp = xdp;
> > > tx_buf->type = ICE_TX_BUF_XSK_TX;
> > > @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> > > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> > > if (!err)
> > > return ICE_XDP_REDIR;
> > > - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> > > + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> > > + err == -ENOBUFS)
> > > result = ICE_XDP_EXIT;
> > > else
> > > result = ICE_XDP_CONSUMED;
> > > @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> > > */
> > > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > {
> > > + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> > > unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> > > - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> > > u32 ntc = rx_ring->next_to_clean;
> > > u32 ntu = rx_ring->next_to_use;
> > > struct xdp_buff *first = NULL;
> > > @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > rx_ring->next_to_clean = ntc;
> > > entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> > > if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> > > - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> > > + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> > > + entries_to_alloc);
> > >
> > > ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> > > ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> > > @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > /**
> > > * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @desc: AF_XDP descriptor to pull the DMA address and length from
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > > +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> > > unsigned int *total_bytes)
> > > {
> > > struct ice_tx_desc *tx_desc;
> > > dma_addr_t dma;
> > >
> > > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> > > + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> > > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
> > >
> > > tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> > > tx_desc->buf_addr = cpu_to_le64(dma);
> > > @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > > /**
> > > * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > > +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool,
> > > + struct xdp_desc *descs,
> > > unsigned int *total_bytes)
> > > {
> > > u16 ntu = xdp_ring->next_to_use;
> > > @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > > loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> > > dma_addr_t dma;
> > >
> > > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> > > + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> > > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
> > >
> > > tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> > > tx_desc->buf_addr = cpu_to_le64(dma);
> > > @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > > /**
> > > * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > > * @nb_pkts: count of packets to be send
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > > - u32 nb_pkts, unsigned int *total_bytes)
> > > +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool,
> > > + struct xdp_desc *descs, u32 nb_pkts,
> > > + unsigned int *total_bytes)
> > > {
> > > u32 batched, leftover, i;
> > >
> > > batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> > > leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> > > for (i = 0; i < batched; i += PKTS_PER_BATCH)
> > > - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> > > + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > > for (; i < batched + leftover; i++)
> > > - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> > > + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > > }
> > >
> > > /**
> > > @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> > > */
> > > bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > > {
> > > - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> > > + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> > > + struct xdp_desc *descs = xsk_pool->tx_descs;
> > > u32 nb_pkts, nb_processed = 0;
> > > unsigned int total_bytes = 0;
> > > int budget;
> > > @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > > budget = ICE_DESC_UNUSED(xdp_ring);
> > > budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
> > >
> > > - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> > > + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> > > - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> > > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> > > + &total_bytes);
> > > xdp_ring->next_to_use = 0;
> > > }
> > >
> > > - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> > > - &total_bytes);
> > > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> > > + nb_pkts - nb_processed, &total_bytes);
> > >
> > > ice_set_rs_bit(xdp_ring);
> > > ice_xdp_ring_update_tail(xdp_ring);
> > > ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
> > >
> > > - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> > > - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> > > + if (xsk_uses_need_wakeup(xsk_pool))
> > > + xsk_set_tx_need_wakeup(xsk_pool);
> > >
> > > return nb_pkts < budget;
> > > }
> > > @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
> > >
> > > ring = vsi->rx_rings[queue_id]->xdp_ring;
> > >
> > > - if (!ring->xsk_pool)
> > > + if (!READ_ONCE(ring->xsk_pool))
> > > return -EINVAL;
> > >
> > > /* The idea here is that if NAPI is running, mark a miss, so
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > index 6fa181f080ef..4cd2d62a0836 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> > > u16 qid);
> > > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> > > int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> > > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> > > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count);
> > > bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> > > void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> > > void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> > > @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
> > >
> > > static inline bool
> > > ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> > > + struct xsk_buff_pool __always_unused *xsk_pool,
> > > u16 __always_unused count)
> > > {
> > > return false;
> > > --
> > > 2.34.1
> > >
> > >
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool
@ 2024-06-04 11:12 ` Maciej Fijalkowski
0 siblings, 0 replies; 46+ messages in thread
From: Maciej Fijalkowski @ 2024-06-04 11:12 UTC (permalink / raw)
To: Nelson, Shannon
Cc: intel-wired-lan, netdev, anthony.l.nguyen, magnus.karlsson,
michal.kubiak, larysa.zaremba
On Tue, Jun 04, 2024 at 12:52:16PM +0200, Maciej Fijalkowski wrote:
> On Fri, May 31, 2024 at 04:49:05PM -0700, Nelson, Shannon wrote:
> > On 5/29/2024 4:23 AM, Maciej Fijalkowski wrote:
> > >
> > > xsk_buff_pool pointers that ice ring structs hold are updated via
> > > ndo_bpf that is executed in process context while it can be read by
> > > remote CPU at the same time within NAPI poll. Use synchronize_net()
> > > after pointer update and {READ,WRITE}_ONCE() when working with mentioned
> > > pointer.
> > >
> > > Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/ice/ice.h | 6 +-
> > > drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
> > > drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
> > > drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
> > > drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
> > > drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> > > 6 files changed, 59 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> > > index da8c8afebc93..701a61d791dd 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -771,12 +771,12 @@ static inline struct xsk_buff_pool *ice_get_xp_from_qid(struct ice_vsi *vsi,
> > > * Returns a pointer to xsk_buff_pool structure if there is a buffer pool
> > > * present, NULL otherwise.
> > > */
> > > -static inline struct xsk_buff_pool *ice_xsk_pool(struct ice_rx_ring *ring)
> > > +static inline void ice_xsk_pool(struct ice_rx_ring *ring)
> >
> > Since this patch is changing the logic here you mighht want to change the
> > name of the function. Instead of getting a pointer with no side effects it
> > is now doing something, so a better name might be ice_set_xsk_pool() to
> > reflect the resulting side effect.
>
> Makes sense, plus the function description needs some adjustment. Will fix
> on v3.
Having second thought on this, I'll go only with
s/ice_xsk_pool/ice_rx_xsk_pool/ to align with Tx side. Tx side also lacks
the 'set' keyword in its naming, but I don't want to touch this here.
I currently don't see a reason for these functions being inlined as we are
not setting xsk pool pointers in the hot path at all, therefore I can go
later on with a patch dedicated for -next that fixes naming and drops the
inlining.
>
> >
> > sln
> >
> > > {
> > > struct ice_vsi *vsi = ring->vsi;
> > > u16 qid = ring->q_index;
> > >
> > > - return ice_get_xp_from_qid(vsi, qid);
> > > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > > }
> > >
> > > /**
> > > @@ -801,7 +801,7 @@ static inline void ice_tx_xsk_pool(struct ice_vsi *vsi, u16 qid)
> > > if (!ring)
> > > return;
> > >
> > > - ring->xsk_pool = ice_get_xp_from_qid(vsi, qid);
> > > + WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
> > > }
> > >
> > > /**
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_base.c b/drivers/net/ethernet/intel/ice/ice_base.c
> > > index 5d396c1a7731..f3dfce136106 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_base.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_base.c
> > > @@ -536,7 +536,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > > return err;
> > > }
> > >
> > > - ring->xsk_pool = ice_xsk_pool(ring);
> > > + ice_xsk_pool(ring);
> > > if (ring->xsk_pool) {
> > > xdp_rxq_info_unreg(&ring->xdp_rxq);
> > >
> > > @@ -597,7 +597,7 @@ static int ice_vsi_cfg_rxq(struct ice_rx_ring *ring)
> > > return 0;
> > > }
> > >
> > > - ok = ice_alloc_rx_bufs_zc(ring, num_bufs);
> > > + ok = ice_alloc_rx_bufs_zc(ring, ring->xsk_pool, num_bufs);
> > > if (!ok) {
> > > u16 pf_q = ring->vsi->rxq_map[ring->q_index];
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index 1b61ca3a6eb6..15a6805ac2a1 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -2946,7 +2946,7 @@ static void ice_vsi_rx_napi_schedule(struct ice_vsi *vsi)
> > > ice_for_each_rxq(vsi, i) {
> > > struct ice_rx_ring *rx_ring = vsi->rx_rings[i];
> > >
> > > - if (rx_ring->xsk_pool)
> > > + if (READ_ONCE(rx_ring->xsk_pool))
> > > napi_schedule(&rx_ring->q_vector->napi);
> > > }
> > > }
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > index 8bb743f78fcb..f4b2b1bca234 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > > @@ -1523,7 +1523,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > > ice_for_each_tx_ring(tx_ring, q_vector->tx) {
> > > bool wd;
> > >
> > > - if (tx_ring->xsk_pool)
> > > + if (READ_ONCE(tx_ring->xsk_pool))
> > > wd = ice_xmit_zc(tx_ring);
> > > else if (ice_ring_is_xdp(tx_ring))
> > > wd = true;
> > > @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
> > > * comparison in the irq context instead of many inside the
> > > * ice_clean_rx_irq function and makes the codebase cleaner.
> > > */
> > > - cleaned = rx_ring->xsk_pool ?
> > > + cleaned = READ_ONCE(rx_ring->xsk_pool) ?
> > > ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
> > > ice_clean_rx_irq(rx_ring, budget_per_ring);
> > > work_done += cleaned;
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > index 8c5006f37310..693f0e3a37ce 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > > @@ -250,6 +250,8 @@ static int ice_qp_ena(struct ice_vsi *vsi, u16 q_idx)
> > > ice_qvec_toggle_napi(vsi, q_vector, true);
> > > ice_qvec_ena_irq(vsi, q_vector);
> > >
> > > + /* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
> > > + synchronize_net();
> > > netif_tx_start_queue(netdev_get_tx_queue(vsi->netdev, q_idx));
> > > netif_carrier_on(vsi->netdev);
> > > clear_bit(ICE_CFG_BUSY, vsi->state);
> > > @@ -461,6 +463,7 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > > /**
> > > * __ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > > * @rx_ring: Rx ring
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > > * @count: The number of buffers to allocate
> > > *
> > > * Place the @count of descriptors onto Rx ring. Handle the ring wrap
> > > @@ -469,7 +472,8 @@ static u16 ice_fill_rx_descs(struct xsk_buff_pool *pool, struct xdp_buff **xdp,
> > > *
> > > * Returns true if all allocations were successful, false if any fail.
> > > */
> > > -static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > +static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count)
> > > {
> > > u32 nb_buffs_extra = 0, nb_buffs = 0;
> > > union ice_32b_rx_flex_desc *rx_desc;
> > > @@ -481,8 +485,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > xdp = ice_xdp_buf(rx_ring, ntu);
> > >
> > > if (ntu + count >= rx_ring->count) {
> > > - nb_buffs_extra = ice_fill_rx_descs(rx_ring->xsk_pool, xdp,
> > > - rx_desc,
> > > + nb_buffs_extra = ice_fill_rx_descs(xsk_pool, xdp, rx_desc,
> > > rx_ring->count - ntu);
> > > if (nb_buffs_extra != rx_ring->count - ntu) {
> > > ntu += nb_buffs_extra;
> > > @@ -495,7 +498,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > ice_release_rx_desc(rx_ring, 0);
> > > }
> > >
> > > - nb_buffs = ice_fill_rx_descs(rx_ring->xsk_pool, xdp, rx_desc, count);
> > > + nb_buffs = ice_fill_rx_descs(xsk_pool, xdp, rx_desc, count);
> > >
> > > ntu += nb_buffs;
> > > if (ntu == rx_ring->count)
> > > @@ -511,6 +514,7 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > /**
> > > * ice_alloc_rx_bufs_zc - allocate a number of Rx buffers
> > > * @rx_ring: Rx ring
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be filled by HW
> > > * @count: The number of buffers to allocate
> > > *
> > > * Wrapper for internal allocation routine; figure out how many tail
> > > @@ -518,7 +522,8 @@ static bool __ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > *
> > > * Returns true if all calls to internal alloc routine succeeded
> > > */
> > > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count)
> > > {
> > > u16 rx_thresh = ICE_RING_QUARTER(rx_ring);
> > > u16 leftover, i, tail_bumps;
> > > @@ -527,9 +532,9 @@ bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count)
> > > leftover = count - (tail_bumps * rx_thresh);
> > >
> > > for (i = 0; i < tail_bumps; i++)
> > > - if (!__ice_alloc_rx_bufs_zc(rx_ring, rx_thresh))
> > > + if (!__ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, rx_thresh))
> > > return false;
> > > - return __ice_alloc_rx_bufs_zc(rx_ring, leftover);
> > > + return __ice_alloc_rx_bufs_zc(rx_ring, xsk_pool, leftover);
> > > }
> > >
> > > /**
> > > @@ -650,7 +655,7 @@ static u32 ice_clean_xdp_irq_zc(struct ice_tx_ring *xdp_ring)
> > > if (xdp_ring->next_to_clean >= cnt)
> > > xdp_ring->next_to_clean -= cnt;
> > > if (xsk_frames)
> > > - xsk_tx_completed(xdp_ring->xsk_pool, xsk_frames);
> > > + xsk_tx_completed(READ_ONCE(xdp_ring->xsk_pool), xsk_frames);
> > >
> > > return completed_frames;
> > > }
> > > @@ -702,7 +707,8 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
> > > dma_addr_t dma;
> > >
> > > dma = xsk_buff_xdp_get_dma(xdp);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, size);
> > > + xsk_buff_raw_dma_sync_for_device(READ_ONCE(xdp_ring->xsk_pool),
> > > + dma, size);
> > >
> > > tx_buf->xdp = xdp;
> > > tx_buf->type = ICE_TX_BUF_XSK_TX;
> > > @@ -760,7 +766,8 @@ ice_run_xdp_zc(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp,
> > > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> > > if (!err)
> > > return ICE_XDP_REDIR;
> > > - if (xsk_uses_need_wakeup(rx_ring->xsk_pool) && err == -ENOBUFS)
> > > + if (xsk_uses_need_wakeup(READ_ONCE(rx_ring->xsk_pool)) &&
> > > + err == -ENOBUFS)
> > > result = ICE_XDP_EXIT;
> > > else
> > > result = ICE_XDP_CONSUMED;
> > > @@ -829,8 +836,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
> > > */
> > > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > {
> > > + struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
> > > unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> > > - struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
> > > u32 ntc = rx_ring->next_to_clean;
> > > u32 ntu = rx_ring->next_to_use;
> > > struct xdp_buff *first = NULL;
> > > @@ -942,7 +949,8 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > rx_ring->next_to_clean = ntc;
> > > entries_to_alloc = ICE_RX_DESC_UNUSED(rx_ring);
> > > if (entries_to_alloc > ICE_RING_QUARTER(rx_ring))
> > > - failure |= !ice_alloc_rx_bufs_zc(rx_ring, entries_to_alloc);
> > > + failure |= !ice_alloc_rx_bufs_zc(rx_ring, xsk_pool,
> > > + entries_to_alloc);
> > >
> > > ice_finalize_xdp_rx(xdp_ring, xdp_xmit, 0);
> > > ice_update_rx_ring_stats(rx_ring, total_rx_packets, total_rx_bytes);
> > > @@ -965,17 +973,19 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
> > > /**
> > > * ice_xmit_pkt - produce a single HW Tx descriptor out of AF_XDP descriptor
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptor on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @desc: AF_XDP descriptor to pull the DMA address and length from
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > > +static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool, struct xdp_desc *desc,
> > > unsigned int *total_bytes)
> > > {
> > > struct ice_tx_desc *tx_desc;
> > > dma_addr_t dma;
> > >
> > > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, desc->addr);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, desc->len);
> > > + dma = xsk_buff_raw_get_dma(xsk_pool, desc->addr);
> > > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, desc->len);
> > >
> > > tx_desc = ICE_TX_DESC(xdp_ring, xdp_ring->next_to_use++);
> > > tx_desc->buf_addr = cpu_to_le64(dma);
> > > @@ -988,10 +998,13 @@ static void ice_xmit_pkt(struct ice_tx_ring *xdp_ring, struct xdp_desc *desc,
> > > /**
> > > * ice_xmit_pkt_batch - produce a batch of HW Tx descriptors out of AF_XDP descriptors
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > > +static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool,
> > > + struct xdp_desc *descs,
> > > unsigned int *total_bytes)
> > > {
> > > u16 ntu = xdp_ring->next_to_use;
> > > @@ -1001,8 +1014,8 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > > loop_unrolled_for(i = 0; i < PKTS_PER_BATCH; i++) {
> > > dma_addr_t dma;
> > >
> > > - dma = xsk_buff_raw_get_dma(xdp_ring->xsk_pool, descs[i].addr);
> > > - xsk_buff_raw_dma_sync_for_device(xdp_ring->xsk_pool, dma, descs[i].len);
> > > + dma = xsk_buff_raw_get_dma(xsk_pool, descs[i].addr);
> > > + xsk_buff_raw_dma_sync_for_device(xsk_pool, dma, descs[i].len);
> > >
> > > tx_desc = ICE_TX_DESC(xdp_ring, ntu++);
> > > tx_desc->buf_addr = cpu_to_le64(dma);
> > > @@ -1018,21 +1031,24 @@ static void ice_xmit_pkt_batch(struct ice_tx_ring *xdp_ring, struct xdp_desc *de
> > > /**
> > > * ice_fill_tx_hw_ring - produce the number of Tx descriptors onto ring
> > > * @xdp_ring: XDP ring to produce the HW Tx descriptors on
> > > + * @xsk_pool: XSK buffer pool to pick buffers to be consumed by HW
> > > * @descs: AF_XDP descriptors to pull the DMA addresses and lengths from
> > > * @nb_pkts: count of packets to be send
> > > * @total_bytes: bytes accumulator that will be used for stats update
> > > */
> > > -static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *descs,
> > > - u32 nb_pkts, unsigned int *total_bytes)
> > > +static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring,
> > > + struct xsk_buff_pool *xsk_pool,
> > > + struct xdp_desc *descs, u32 nb_pkts,
> > > + unsigned int *total_bytes)
> > > {
> > > u32 batched, leftover, i;
> > >
> > > batched = ALIGN_DOWN(nb_pkts, PKTS_PER_BATCH);
> > > leftover = nb_pkts & (PKTS_PER_BATCH - 1);
> > > for (i = 0; i < batched; i += PKTS_PER_BATCH)
> > > - ice_xmit_pkt_batch(xdp_ring, &descs[i], total_bytes);
> > > + ice_xmit_pkt_batch(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > > for (; i < batched + leftover; i++)
> > > - ice_xmit_pkt(xdp_ring, &descs[i], total_bytes);
> > > + ice_xmit_pkt(xdp_ring, xsk_pool, &descs[i], total_bytes);
> > > }
> > >
> > > /**
> > > @@ -1043,7 +1059,8 @@ static void ice_fill_tx_hw_ring(struct ice_tx_ring *xdp_ring, struct xdp_desc *d
> > > */
> > > bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > > {
> > > - struct xdp_desc *descs = xdp_ring->xsk_pool->tx_descs;
> > > + struct xsk_buff_pool *xsk_pool = READ_ONCE(xdp_ring->xsk_pool);
> > > + struct xdp_desc *descs = xsk_pool->tx_descs;
> > > u32 nb_pkts, nb_processed = 0;
> > > unsigned int total_bytes = 0;
> > > int budget;
> > > @@ -1057,25 +1074,26 @@ bool ice_xmit_zc(struct ice_tx_ring *xdp_ring)
> > > budget = ICE_DESC_UNUSED(xdp_ring);
> > > budget = min_t(u16, budget, ICE_RING_QUARTER(xdp_ring));
> > >
> > > - nb_pkts = xsk_tx_peek_release_desc_batch(xdp_ring->xsk_pool, budget);
> > > + nb_pkts = xsk_tx_peek_release_desc_batch(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;
> > > - ice_fill_tx_hw_ring(xdp_ring, descs, nb_processed, &total_bytes);
> > > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, descs, nb_processed,
> > > + &total_bytes);
> > > xdp_ring->next_to_use = 0;
> > > }
> > >
> > > - ice_fill_tx_hw_ring(xdp_ring, &descs[nb_processed], nb_pkts - nb_processed,
> > > - &total_bytes);
> > > + ice_fill_tx_hw_ring(xdp_ring, xsk_pool, &descs[nb_processed],
> > > + nb_pkts - nb_processed, &total_bytes);
> > >
> > > ice_set_rs_bit(xdp_ring);
> > > ice_xdp_ring_update_tail(xdp_ring);
> > > ice_update_tx_ring_stats(xdp_ring, nb_pkts, total_bytes);
> > >
> > > - if (xsk_uses_need_wakeup(xdp_ring->xsk_pool))
> > > - xsk_set_tx_need_wakeup(xdp_ring->xsk_pool);
> > > + if (xsk_uses_need_wakeup(xsk_pool))
> > > + xsk_set_tx_need_wakeup(xsk_pool);
> > >
> > > return nb_pkts < budget;
> > > }
> > > @@ -1108,7 +1126,7 @@ ice_xsk_wakeup(struct net_device *netdev, u32 queue_id,
> > >
> > > ring = vsi->rx_rings[queue_id]->xdp_ring;
> > >
> > > - if (!ring->xsk_pool)
> > > + if (!READ_ONCE(ring->xsk_pool))
> > > return -EINVAL;
> > >
> > > /* The idea here is that if NAPI is running, mark a miss, so
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > index 6fa181f080ef..4cd2d62a0836 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
> > > @@ -22,7 +22,8 @@ int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
> > > u16 qid);
> > > int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
> > > int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
> > > -bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring, u16 count);
> > > +bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
> > > + struct xsk_buff_pool *xsk_pool, u16 count);
> > > bool ice_xsk_any_rx_ring_ena(struct ice_vsi *vsi);
> > > void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring);
> > > void ice_xsk_clean_xdp_ring(struct ice_tx_ring *xdp_ring);
> > > @@ -51,6 +52,7 @@ ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
> > >
> > > static inline bool
> > > ice_alloc_rx_bufs_zc(struct ice_rx_ring __always_unused *rx_ring,
> > > + struct xsk_buff_pool __always_unused *xsk_pool,
> > > u16 __always_unused count)
> > > {
> > > return false;
> > > --
> > > 2.34.1
> > >
> > >
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool
2024-05-29 11:23 ` [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,r}x_ring::xsk_pool Maciej Fijalkowski
@ 2024-06-04 6:50 ` Rout, ChandanX
-1 siblings, 0 replies; 46+ messages in thread
From: Rout, ChandanX @ 2024-06-04 6:50 UTC (permalink / raw)
To: Fijalkowski, Maciej, intel-wired-lan@lists.osuosl.org
Cc: Zaremba, Larysa, netdev@vger.kernel.org, Pandey, Atul,
Kubiak, Michal, Nguyen, Anthony L, Nagraj, Shravan,
Kuruvinakunnel, George, Karlsson, Magnus
>-----Original Message-----
>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>Fijalkowski, Maciej
>Sent: Wednesday, May 29, 2024 4:54 PM
>To: intel-wired-lan@lists.osuosl.org
>Cc: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Zaremba, Larysa
><larysa.zaremba@intel.com>; netdev@vger.kernel.org; Kubiak, Michal
><michal.kubiak@intel.com>; Nguyen, Anthony L
><anthony.l.nguyen@intel.com>; Karlsson, Magnus
><magnus.karlsson@intel.com>
>Subject: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,
>r}x_ring::xsk_pool
>
>xsk_buff_pool pointers that ice ring structs hold are updated via ndo_bpf that is
>executed in process context while it can be read by remote CPU at the same
>time within NAPI poll. Use synchronize_net() after pointer update and
>{READ,WRITE}_ONCE() when working with mentioned pointer.
>
>Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
>Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>---
> drivers/net/ethernet/intel/ice/ice.h | 6 +-
> drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
>drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
>drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
>drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
>drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> 6 files changed, 59 insertions(+), 39 deletions(-)
>
Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
^ permalink raw reply [flat|nested] 46+ messages in thread* RE: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool
@ 2024-06-04 6:50 ` Rout, ChandanX
0 siblings, 0 replies; 46+ messages in thread
From: Rout, ChandanX @ 2024-06-04 6:50 UTC (permalink / raw)
To: Fijalkowski, Maciej, intel-wired-lan@lists.osuosl.org
Cc: Zaremba, Larysa, netdev@vger.kernel.org, Kubiak, Michal,
Nguyen, Anthony L, Karlsson, Magnus, Kuruvinakunnel, George,
Pandey, Atul, Nagaraju, Shwetha, Nagraj, Shravan
>-----Original Message-----
>From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>Fijalkowski, Maciej
>Sent: Wednesday, May 29, 2024 4:54 PM
>To: intel-wired-lan@lists.osuosl.org
>Cc: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; Zaremba, Larysa
><larysa.zaremba@intel.com>; netdev@vger.kernel.org; Kubiak, Michal
><michal.kubiak@intel.com>; Nguyen, Anthony L
><anthony.l.nguyen@intel.com>; Karlsson, Magnus
><magnus.karlsson@intel.com>
>Subject: [Intel-wired-lan] [PATCH v2 iwl-net 6/8] ice: improve updating ice_{t,
>r}x_ring::xsk_pool
>
>xsk_buff_pool pointers that ice ring structs hold are updated via ndo_bpf that is
>executed in process context while it can be read by remote CPU at the same
>time within NAPI poll. Use synchronize_net() after pointer update and
>{READ,WRITE}_ONCE() when working with mentioned pointer.
>
>Fixes: 2d4238f55697 ("ice: Add support for AF_XDP")
>Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>---
> drivers/net/ethernet/intel/ice/ice.h | 6 +-
> drivers/net/ethernet/intel/ice/ice_base.c | 4 +-
>drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
>drivers/net/ethernet/intel/ice/ice_txrx.c | 4 +-
>drivers/net/ethernet/intel/ice/ice_xsk.c | 78 ++++++++++++++---------
>drivers/net/ethernet/intel/ice/ice_xsk.h | 4 +-
> 6 files changed, 59 insertions(+), 39 deletions(-)
>
Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
^ permalink raw reply [flat|nested] 46+ messages in thread