| Message ID | 20221006130948.16053-1-jan.sokolowski@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net,v5] i40e: Fix DMA mappings leak | expand |
Add Maciej... However, you should first give responses to the previous patch [1] instead of sending a new version without acknowledging any feedback on the other patch (and not including the person(s) asking questions on the updated version) [1] https://lore.kernel.org/netdev/YzV9gXMMPMjmQWDE@boxer/ On 10/6/2022 6:09 AM, Jan Sokolowski wrote: > During reallocation of RX buffers, new DMA mappings are created for > those buffers. New buffers with different RX ring count should > substitute older ones, but those buffers were freed in > i40e_configure_rx_ring and reallocated again with i40e_alloc_rx_bi, > thus kfree on rx_bi caused leak of already mapped DMA. > > Reallocate ZC with rx_bi_zc struct, when BPF program loads. Reallocate > back to rx_bi, when BPF program unloads. > > If BPF program is loaded/unloaded and XSK pools are created, reallocate > RX queues accordingly in XSP_SETUP_XSK_POOL handler. > > steps for reproduction: > while : > do > for ((i=0; i<=8160; i=i+32)) > do > ethtool -G enp130s0f0 rx $i tx $i > sleep 0.5 > ethtool -g enp130s0f0 > done > done > > Fixes: be1222b585fd ("i40e: Separate kernel allocated rx_bi rings from AF_XDP rings") > Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> > --- > v2: Fixed improper kerneldoc that resulted in a warning > v3: Applied commit msg fixes reported during a review > v4: Applied i40e_xsk.c fixes reported during a review > v5: applied commit message and general fixes reported by > Maciej Fijalkowski's review. > .../net/ethernet/intel/i40e/i40e_ethtool.c | 3 - > drivers/net/ethernet/intel/i40e/i40e_main.c | 15 +++-- > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 13 ++-- > drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 - > drivers/net/ethernet/intel/i40e/i40e_xsk.c | 67 ++++++++++++++++--- > drivers/net/ethernet/intel/i40e/i40e_xsk.h | 2 +- > 6 files changed, 73 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > index ae51901e671d..4a6a6e48c615 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > @@ -2181,9 +2181,6 @@ static int i40e_set_ringparam(struct net_device *netdev, > */ > rx_rings[i].tail = hw->hw_addr + I40E_PRTGEN_STATUS; > err = i40e_setup_rx_descriptors(&rx_rings[i]); > - if (err) > - goto rx_unwind; > - err = i40e_alloc_rx_bi(&rx_rings[i]); > if (err) > goto rx_unwind; > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index 6ecb6013d97d..cbe2e0592519 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -3566,12 +3566,8 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) > if (ring->vsi->type == I40E_VSI_MAIN) > xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq); > > - kfree(ring->rx_bi); > ring->xsk_pool = i40e_xsk_pool(ring); > if (ring->xsk_pool) { > - ret = i40e_alloc_rx_bi_zc(ring); > - if (ret) > - return ret; > ring->rx_buf_len = > xsk_pool_get_rx_frame_size(ring->xsk_pool); > /* For AF_XDP ZC, we disallow packets to span on > @@ -3589,9 +3585,6 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) > ring->queue_index); > > } else { > - ret = i40e_alloc_rx_bi(ring); > - if (ret) > - return ret; > ring->rx_buf_len = vsi->rx_buf_len; > if (ring->vsi->type == I40E_VSI_MAIN) { > ret = xdp_rxq_info_reg_mem_model(&ring->xdp_rxq, > @@ -13405,6 +13398,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, > i40e_reset_and_rebuild(pf, true, true); > } > > + if (!i40e_enabled_xdp_vsi(vsi) && prog) > + if (i40e_realloc_rx_bi_zc(vsi, true)) > + return -ENOMEM; > + else if (i40e_enabled_xdp_vsi(vsi) && !prog) > + if (i40e_realloc_rx_bi_zc(vsi, false)) > + return -ENOMEM; > + > for (i = 0; i < vsi->num_queue_pairs; i++) > WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog); > > @@ -13637,6 +13637,7 @@ int i40e_queue_pair_disable(struct i40e_vsi *vsi, int queue_pair) > > i40e_queue_pair_disable_irq(vsi, queue_pair); > err = i40e_queue_pair_toggle_rings(vsi, queue_pair, false /* off */); > + i40e_clean_rx_ring(vsi->rx_rings[queue_pair]); > i40e_queue_pair_toggle_napi(vsi, queue_pair, false /* off */); > i40e_queue_pair_clean_rings(vsi, queue_pair); > i40e_queue_pair_reset_stats(vsi, queue_pair); > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > index 69e67eb6aea7..b97c95f89fa0 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > @@ -1457,14 +1457,6 @@ int i40e_setup_tx_descriptors(struct i40e_ring *tx_ring) > return -ENOMEM; > } > > -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring) > -{ > - unsigned long sz = sizeof(*rx_ring->rx_bi) * rx_ring->count; > - > - rx_ring->rx_bi = kzalloc(sz, GFP_KERNEL); > - return rx_ring->rx_bi ? 0 : -ENOMEM; > -} > - > static void i40e_clear_rx_bi(struct i40e_ring *rx_ring) > { > memset(rx_ring->rx_bi, 0, sizeof(*rx_ring->rx_bi) * rx_ring->count); > @@ -1593,6 +1585,11 @@ int i40e_setup_rx_descriptors(struct i40e_ring *rx_ring) > > rx_ring->xdp_prog = rx_ring->vsi->xdp_prog; > > + rx_ring->rx_bi = > + kcalloc(rx_ring->count, sizeof(*rx_ring->rx_bi), GFP_KERNEL); > + if (!rx_ring->rx_bi) > + return -ENOMEM; > + > return 0; > } > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h > index 41f86e9535a0..768290dc6f48 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h > @@ -469,7 +469,6 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size); > bool __i40e_chk_linearize(struct sk_buff *skb); > int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames, > u32 flags); > -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring); > > /** > * i40e_get_head - Retrieve head from head writeback > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c > index 6d4009e0cbd6..cd7b52fb6b46 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c > @@ -10,14 +10,6 @@ > #include "i40e_txrx_common.h" > #include "i40e_xsk.h" > > -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring) > -{ > - unsigned long sz = sizeof(*rx_ring->rx_bi_zc) * rx_ring->count; > - > - rx_ring->rx_bi_zc = kzalloc(sz, GFP_KERNEL); > - return rx_ring->rx_bi_zc ? 0 : -ENOMEM; > -} > - > void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring) > { > memset(rx_ring->rx_bi_zc, 0, > @@ -29,6 +21,58 @@ static struct xdp_buff **i40e_rx_bi(struct i40e_ring *rx_ring, u32 idx) > return &rx_ring->rx_bi_zc[idx]; > } > > +/** > + * i40e_realloc_rx_xdp_bi - reallocate SW ring for either XSK or normal buffer > + * @rx_ring: Current rx ring > + * @pool_present: is pool for XSK present > + * > + * Try allocating memory and return ENOMEM, if failed to allocate. > + * If allocation was successful, substitute buffer with allocated one. > + * Returns 0 on success, negative on failure > + */ > +static int i40e_realloc_rx_xdp_bi(struct i40e_ring *rx_ring, bool pool_present) > +{ > + size_t elem_size = pool_present ? sizeof(*rx_ring->rx_bi_zc) : > + sizeof(*rx_ring->rx_bi); > + void *sw_ring = kcalloc(rx_ring->count, elem_size, GFP_KERNEL); > + > + if (!sw_ring) > + return -ENOMEM; > + > + if (pool_present) { > + kfree(rx_ring->rx_bi); > + rx_ring->rx_bi = NULL; > + rx_ring->rx_bi_zc = sw_ring; > + } else { > + kfree(rx_ring->rx_bi_zc); > + rx_ring->rx_bi_zc = NULL; > + rx_ring->rx_bi = sw_ring; > + } > + return 0; > +} > + > +/** > + * i40e_realloc_rx_bi_zc - reallocate rx SW rings > + * @vsi: Current VSI > + * @zc: is zero copy set > + * > + * Reallocate buffer for rx_rings that might be used by XSK. > + * XDP requires more memory, than rx_buf provides. > + * Returns 0 on success, negative on failure > + */ > +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc) > +{ > + struct i40e_ring *rx_ring; > + unsigned long q; > + > + for_each_set_bit(q, vsi->af_xdp_zc_qps, vsi->alloc_queue_pairs) { > + rx_ring = vsi->rx_rings[q]; > + if (i40e_realloc_rx_xdp_bi(rx_ring, zc)) > + return -ENOMEM; > + } > + return 0; > +} > + > /** > * i40e_xsk_pool_enable - Enable/associate an AF_XDP buffer pool to a > * certain ring/qid > @@ -69,6 +113,10 @@ static int i40e_xsk_pool_enable(struct i40e_vsi *vsi, > if (err) > return err; > > + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], true); > + if (err) > + return err; > + > err = i40e_queue_pair_enable(vsi, qid); > if (err) > return err; > @@ -113,6 +161,9 @@ static int i40e_xsk_pool_disable(struct i40e_vsi *vsi, u16 qid) > xsk_pool_dma_unmap(pool, I40E_RX_DMA_ATTR); > > if (if_running) { > + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], false); > + if (err) > + return err; > err = i40e_queue_pair_enable(vsi, qid); > if (err) > return err; > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.h b/drivers/net/ethernet/intel/i40e/i40e_xsk.h > index bb962987f300..821df248f8be 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.h > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.h > @@ -32,7 +32,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget); > > bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi, struct i40e_ring *tx_ring); > int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags); > -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring); > +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc); > void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring); > > #endif /* _I40E_XSK_H_ */
On Thu, Oct 06, 2022 at 11:24:52AM -0700, Tony Nguyen wrote: > Add Maciej... > > However, you should first give responses to the previous patch [1] instead > of sending a new version without acknowledging any feedback on the other > patch (and not including the person(s) asking questions on the updated > version) > > [1] https://lore.kernel.org/netdev/YzV9gXMMPMjmQWDE@boxer/ Yeah just answering to my response with "ok maciej i'll do it in v5" or "maciej you suck and i disagree" would be better than what Tony described above:) > > On 10/6/2022 6:09 AM, Jan Sokolowski wrote: > > During reallocation of RX buffers, new DMA mappings are created for > > those buffers. New buffers with different RX ring count should > > substitute older ones, but those buffers were freed in > > i40e_configure_rx_ring and reallocated again with i40e_alloc_rx_bi, > > thus kfree on rx_bi caused leak of already mapped DMA. > > > > Reallocate ZC with rx_bi_zc struct, when BPF program loads. Reallocate > > back to rx_bi, when BPF program unloads. > > > > If BPF program is loaded/unloaded and XSK pools are created, reallocate > > RX queues accordingly in XSP_SETUP_XSK_POOL handler. > > > > steps for reproduction: Reproduction of what?:) It would be better to start off with this and problem statement, something like "if you do the steps below, splat xyz occurs [include the splat]". Then say it is because of yadda yadda. In order to fix this, do xyz. > > while : > > do > > for ((i=0; i<=8160; i=i+32)) > > do > > ethtool -G enp130s0f0 rx $i tx $i > > sleep 0.5 > > ethtool -g enp130s0f0 > > done > > done > > > > Fixes: be1222b585fd ("i40e: Separate kernel allocated rx_bi rings from AF_XDP rings") > > Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> Why did you strip tested by tag? With the code part, I think I'm fine with the current state. > > --- > > v2: Fixed improper kerneldoc that resulted in a warning > > v3: Applied commit msg fixes reported during a review > > v4: Applied i40e_xsk.c fixes reported during a review > > v5: applied commit message and general fixes reported by > > Maciej Fijalkowski's review. > > .../net/ethernet/intel/i40e/i40e_ethtool.c | 3 - > > drivers/net/ethernet/intel/i40e/i40e_main.c | 15 +++-- > > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 13 ++-- > > drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 - > > drivers/net/ethernet/intel/i40e/i40e_xsk.c | 67 ++++++++++++++++--- > > drivers/net/ethernet/intel/i40e/i40e_xsk.h | 2 +- > > 6 files changed, 73 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > index ae51901e671d..4a6a6e48c615 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c > > @@ -2181,9 +2181,6 @@ static int i40e_set_ringparam(struct net_device *netdev, > > */ > > rx_rings[i].tail = hw->hw_addr + I40E_PRTGEN_STATUS; > > err = i40e_setup_rx_descriptors(&rx_rings[i]); > > - if (err) > > - goto rx_unwind; > > - err = i40e_alloc_rx_bi(&rx_rings[i]); > > if (err) > > goto rx_unwind; > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > > index 6ecb6013d97d..cbe2e0592519 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > > @@ -3566,12 +3566,8 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) > > if (ring->vsi->type == I40E_VSI_MAIN) > > xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq); > > - kfree(ring->rx_bi); > > ring->xsk_pool = i40e_xsk_pool(ring); > > if (ring->xsk_pool) { > > - ret = i40e_alloc_rx_bi_zc(ring); > > - if (ret) > > - return ret; > > ring->rx_buf_len = > > xsk_pool_get_rx_frame_size(ring->xsk_pool); > > /* For AF_XDP ZC, we disallow packets to span on > > @@ -3589,9 +3585,6 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) > > ring->queue_index); > > } else { > > - ret = i40e_alloc_rx_bi(ring); > > - if (ret) > > - return ret; > > ring->rx_buf_len = vsi->rx_buf_len; > > if (ring->vsi->type == I40E_VSI_MAIN) { > > ret = xdp_rxq_info_reg_mem_model(&ring->xdp_rxq, > > @@ -13405,6 +13398,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, > > i40e_reset_and_rebuild(pf, true, true); > > } > > + if (!i40e_enabled_xdp_vsi(vsi) && prog) > > + if (i40e_realloc_rx_bi_zc(vsi, true)) > > + return -ENOMEM; > > + else if (i40e_enabled_xdp_vsi(vsi) && !prog) > > + if (i40e_realloc_rx_bi_zc(vsi, false)) > > + return -ENOMEM; > > + > > for (i = 0; i < vsi->num_queue_pairs; i++) > > WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog); > > @@ -13637,6 +13637,7 @@ int i40e_queue_pair_disable(struct i40e_vsi *vsi, int queue_pair) > > i40e_queue_pair_disable_irq(vsi, queue_pair); > > err = i40e_queue_pair_toggle_rings(vsi, queue_pair, false /* off */); > > + i40e_clean_rx_ring(vsi->rx_rings[queue_pair]); > > i40e_queue_pair_toggle_napi(vsi, queue_pair, false /* off */); > > i40e_queue_pair_clean_rings(vsi, queue_pair); > > i40e_queue_pair_reset_stats(vsi, queue_pair); > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > > index 69e67eb6aea7..b97c95f89fa0 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c > > @@ -1457,14 +1457,6 @@ int i40e_setup_tx_descriptors(struct i40e_ring *tx_ring) > > return -ENOMEM; > > } > > -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring) > > -{ > > - unsigned long sz = sizeof(*rx_ring->rx_bi) * rx_ring->count; > > - > > - rx_ring->rx_bi = kzalloc(sz, GFP_KERNEL); > > - return rx_ring->rx_bi ? 0 : -ENOMEM; > > -} > > - > > static void i40e_clear_rx_bi(struct i40e_ring *rx_ring) > > { > > memset(rx_ring->rx_bi, 0, sizeof(*rx_ring->rx_bi) * rx_ring->count); > > @@ -1593,6 +1585,11 @@ int i40e_setup_rx_descriptors(struct i40e_ring *rx_ring) > > rx_ring->xdp_prog = rx_ring->vsi->xdp_prog; > > + rx_ring->rx_bi = > > + kcalloc(rx_ring->count, sizeof(*rx_ring->rx_bi), GFP_KERNEL); > > + if (!rx_ring->rx_bi) > > + return -ENOMEM; > > + > > return 0; > > } > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h > > index 41f86e9535a0..768290dc6f48 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h > > +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h > > @@ -469,7 +469,6 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size); > > bool __i40e_chk_linearize(struct sk_buff *skb); > > int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames, > > u32 flags); > > -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring); > > /** > > * i40e_get_head - Retrieve head from head writeback > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c > > index 6d4009e0cbd6..cd7b52fb6b46 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c > > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c > > @@ -10,14 +10,6 @@ > > #include "i40e_txrx_common.h" > > #include "i40e_xsk.h" > > -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring) > > -{ > > - unsigned long sz = sizeof(*rx_ring->rx_bi_zc) * rx_ring->count; > > - > > - rx_ring->rx_bi_zc = kzalloc(sz, GFP_KERNEL); > > - return rx_ring->rx_bi_zc ? 0 : -ENOMEM; > > -} > > - > > void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring) > > { > > memset(rx_ring->rx_bi_zc, 0, > > @@ -29,6 +21,58 @@ static struct xdp_buff **i40e_rx_bi(struct i40e_ring *rx_ring, u32 idx) > > return &rx_ring->rx_bi_zc[idx]; > > } > > +/** > > + * i40e_realloc_rx_xdp_bi - reallocate SW ring for either XSK or normal buffer > > + * @rx_ring: Current rx ring > > + * @pool_present: is pool for XSK present > > + * > > + * Try allocating memory and return ENOMEM, if failed to allocate. > > + * If allocation was successful, substitute buffer with allocated one. > > + * Returns 0 on success, negative on failure > > + */ > > +static int i40e_realloc_rx_xdp_bi(struct i40e_ring *rx_ring, bool pool_present) > > +{ > > + size_t elem_size = pool_present ? sizeof(*rx_ring->rx_bi_zc) : > > + sizeof(*rx_ring->rx_bi); > > + void *sw_ring = kcalloc(rx_ring->count, elem_size, GFP_KERNEL); > > + > > + if (!sw_ring) > > + return -ENOMEM; > > + > > + if (pool_present) { > > + kfree(rx_ring->rx_bi); > > + rx_ring->rx_bi = NULL; > > + rx_ring->rx_bi_zc = sw_ring; > > + } else { > > + kfree(rx_ring->rx_bi_zc); > > + rx_ring->rx_bi_zc = NULL; > > + rx_ring->rx_bi = sw_ring; > > + } > > + return 0; > > +} > > + > > +/** > > + * i40e_realloc_rx_bi_zc - reallocate rx SW rings > > + * @vsi: Current VSI > > + * @zc: is zero copy set > > + * > > + * Reallocate buffer for rx_rings that might be used by XSK. > > + * XDP requires more memory, than rx_buf provides. > > + * Returns 0 on success, negative on failure > > + */ > > +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc) > > +{ > > + struct i40e_ring *rx_ring; > > + unsigned long q; > > + > > + for_each_set_bit(q, vsi->af_xdp_zc_qps, vsi->alloc_queue_pairs) { > > + rx_ring = vsi->rx_rings[q]; > > + if (i40e_realloc_rx_xdp_bi(rx_ring, zc)) > > + return -ENOMEM; > > + } > > + return 0; > > +} > > + > > /** > > * i40e_xsk_pool_enable - Enable/associate an AF_XDP buffer pool to a > > * certain ring/qid > > @@ -69,6 +113,10 @@ static int i40e_xsk_pool_enable(struct i40e_vsi *vsi, > > if (err) > > return err; > > + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], true); > > + if (err) > > + return err; > > + > > err = i40e_queue_pair_enable(vsi, qid); > > if (err) > > return err; > > @@ -113,6 +161,9 @@ static int i40e_xsk_pool_disable(struct i40e_vsi *vsi, u16 qid) > > xsk_pool_dma_unmap(pool, I40E_RX_DMA_ATTR); > > if (if_running) { > > + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], false); > > + if (err) > > + return err; > > err = i40e_queue_pair_enable(vsi, qid); > > if (err) > > return err; > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.h b/drivers/net/ethernet/intel/i40e/i40e_xsk.h > > index bb962987f300..821df248f8be 100644 > > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.h > > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.h > > @@ -32,7 +32,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget); > > bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi, struct i40e_ring *tx_ring); > > int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags); > > -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring); > > +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc); > > void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring); > > #endif /* _I40E_XSK_H_ */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c index ae51901e671d..4a6a6e48c615 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c @@ -2181,9 +2181,6 @@ static int i40e_set_ringparam(struct net_device *netdev, */ rx_rings[i].tail = hw->hw_addr + I40E_PRTGEN_STATUS; err = i40e_setup_rx_descriptors(&rx_rings[i]); - if (err) - goto rx_unwind; - err = i40e_alloc_rx_bi(&rx_rings[i]); if (err) goto rx_unwind; diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index 6ecb6013d97d..cbe2e0592519 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -3566,12 +3566,8 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) if (ring->vsi->type == I40E_VSI_MAIN) xdp_rxq_info_unreg_mem_model(&ring->xdp_rxq); - kfree(ring->rx_bi); ring->xsk_pool = i40e_xsk_pool(ring); if (ring->xsk_pool) { - ret = i40e_alloc_rx_bi_zc(ring); - if (ret) - return ret; ring->rx_buf_len = xsk_pool_get_rx_frame_size(ring->xsk_pool); /* For AF_XDP ZC, we disallow packets to span on @@ -3589,9 +3585,6 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring) ring->queue_index); } else { - ret = i40e_alloc_rx_bi(ring); - if (ret) - return ret; ring->rx_buf_len = vsi->rx_buf_len; if (ring->vsi->type == I40E_VSI_MAIN) { ret = xdp_rxq_info_reg_mem_model(&ring->xdp_rxq, @@ -13405,6 +13398,13 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, i40e_reset_and_rebuild(pf, true, true); } + if (!i40e_enabled_xdp_vsi(vsi) && prog) + if (i40e_realloc_rx_bi_zc(vsi, true)) + return -ENOMEM; + else if (i40e_enabled_xdp_vsi(vsi) && !prog) + if (i40e_realloc_rx_bi_zc(vsi, false)) + return -ENOMEM; + for (i = 0; i < vsi->num_queue_pairs; i++) WRITE_ONCE(vsi->rx_rings[i]->xdp_prog, vsi->xdp_prog); @@ -13637,6 +13637,7 @@ int i40e_queue_pair_disable(struct i40e_vsi *vsi, int queue_pair) i40e_queue_pair_disable_irq(vsi, queue_pair); err = i40e_queue_pair_toggle_rings(vsi, queue_pair, false /* off */); + i40e_clean_rx_ring(vsi->rx_rings[queue_pair]); i40e_queue_pair_toggle_napi(vsi, queue_pair, false /* off */); i40e_queue_pair_clean_rings(vsi, queue_pair); i40e_queue_pair_reset_stats(vsi, queue_pair); diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c index 69e67eb6aea7..b97c95f89fa0 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c @@ -1457,14 +1457,6 @@ int i40e_setup_tx_descriptors(struct i40e_ring *tx_ring) return -ENOMEM; } -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring) -{ - unsigned long sz = sizeof(*rx_ring->rx_bi) * rx_ring->count; - - rx_ring->rx_bi = kzalloc(sz, GFP_KERNEL); - return rx_ring->rx_bi ? 0 : -ENOMEM; -} - static void i40e_clear_rx_bi(struct i40e_ring *rx_ring) { memset(rx_ring->rx_bi, 0, sizeof(*rx_ring->rx_bi) * rx_ring->count); @@ -1593,6 +1585,11 @@ int i40e_setup_rx_descriptors(struct i40e_ring *rx_ring) rx_ring->xdp_prog = rx_ring->vsi->xdp_prog; + rx_ring->rx_bi = + kcalloc(rx_ring->count, sizeof(*rx_ring->rx_bi), GFP_KERNEL); + if (!rx_ring->rx_bi) + return -ENOMEM; + return 0; } diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h index 41f86e9535a0..768290dc6f48 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h @@ -469,7 +469,6 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size); bool __i40e_chk_linearize(struct sk_buff *skb); int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames, u32 flags); -int i40e_alloc_rx_bi(struct i40e_ring *rx_ring); /** * i40e_get_head - Retrieve head from head writeback diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c index 6d4009e0cbd6..cd7b52fb6b46 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c @@ -10,14 +10,6 @@ #include "i40e_txrx_common.h" #include "i40e_xsk.h" -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring) -{ - unsigned long sz = sizeof(*rx_ring->rx_bi_zc) * rx_ring->count; - - rx_ring->rx_bi_zc = kzalloc(sz, GFP_KERNEL); - return rx_ring->rx_bi_zc ? 0 : -ENOMEM; -} - void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring) { memset(rx_ring->rx_bi_zc, 0, @@ -29,6 +21,58 @@ static struct xdp_buff **i40e_rx_bi(struct i40e_ring *rx_ring, u32 idx) return &rx_ring->rx_bi_zc[idx]; } +/** + * i40e_realloc_rx_xdp_bi - reallocate SW ring for either XSK or normal buffer + * @rx_ring: Current rx ring + * @pool_present: is pool for XSK present + * + * Try allocating memory and return ENOMEM, if failed to allocate. + * If allocation was successful, substitute buffer with allocated one. + * Returns 0 on success, negative on failure + */ +static int i40e_realloc_rx_xdp_bi(struct i40e_ring *rx_ring, bool pool_present) +{ + size_t elem_size = pool_present ? sizeof(*rx_ring->rx_bi_zc) : + sizeof(*rx_ring->rx_bi); + void *sw_ring = kcalloc(rx_ring->count, elem_size, GFP_KERNEL); + + if (!sw_ring) + return -ENOMEM; + + if (pool_present) { + kfree(rx_ring->rx_bi); + rx_ring->rx_bi = NULL; + rx_ring->rx_bi_zc = sw_ring; + } else { + kfree(rx_ring->rx_bi_zc); + rx_ring->rx_bi_zc = NULL; + rx_ring->rx_bi = sw_ring; + } + return 0; +} + +/** + * i40e_realloc_rx_bi_zc - reallocate rx SW rings + * @vsi: Current VSI + * @zc: is zero copy set + * + * Reallocate buffer for rx_rings that might be used by XSK. + * XDP requires more memory, than rx_buf provides. + * Returns 0 on success, negative on failure + */ +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc) +{ + struct i40e_ring *rx_ring; + unsigned long q; + + for_each_set_bit(q, vsi->af_xdp_zc_qps, vsi->alloc_queue_pairs) { + rx_ring = vsi->rx_rings[q]; + if (i40e_realloc_rx_xdp_bi(rx_ring, zc)) + return -ENOMEM; + } + return 0; +} + /** * i40e_xsk_pool_enable - Enable/associate an AF_XDP buffer pool to a * certain ring/qid @@ -69,6 +113,10 @@ static int i40e_xsk_pool_enable(struct i40e_vsi *vsi, if (err) return err; + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], true); + if (err) + return err; + err = i40e_queue_pair_enable(vsi, qid); if (err) return err; @@ -113,6 +161,9 @@ static int i40e_xsk_pool_disable(struct i40e_vsi *vsi, u16 qid) xsk_pool_dma_unmap(pool, I40E_RX_DMA_ATTR); if (if_running) { + err = i40e_realloc_rx_xdp_bi(vsi->rx_rings[qid], false); + if (err) + return err; err = i40e_queue_pair_enable(vsi, qid); if (err) return err; diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.h b/drivers/net/ethernet/intel/i40e/i40e_xsk.h index bb962987f300..821df248f8be 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.h +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.h @@ -32,7 +32,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget); bool i40e_clean_xdp_tx_irq(struct i40e_vsi *vsi, struct i40e_ring *tx_ring); int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags); -int i40e_alloc_rx_bi_zc(struct i40e_ring *rx_ring); +int i40e_realloc_rx_bi_zc(struct i40e_vsi *vsi, bool zc); void i40e_clear_rx_bi_zc(struct i40e_ring *rx_ring); #endif /* _I40E_XSK_H_ */
During reallocation of RX buffers, new DMA mappings are created for those buffers. New buffers with different RX ring count should substitute older ones, but those buffers were freed in i40e_configure_rx_ring and reallocated again with i40e_alloc_rx_bi, thus kfree on rx_bi caused leak of already mapped DMA. Reallocate ZC with rx_bi_zc struct, when BPF program loads. Reallocate back to rx_bi, when BPF program unloads. If BPF program is loaded/unloaded and XSK pools are created, reallocate RX queues accordingly in XSP_SETUP_XSK_POOL handler. steps for reproduction: while : do for ((i=0; i<=8160; i=i+32)) do ethtool -G enp130s0f0 rx $i tx $i sleep 0.5 ethtool -g enp130s0f0 done done Fixes: be1222b585fd ("i40e: Separate kernel allocated rx_bi rings from AF_XDP rings") Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com> --- v2: Fixed improper kerneldoc that resulted in a warning v3: Applied commit msg fixes reported during a review v4: Applied i40e_xsk.c fixes reported during a review v5: applied commit message and general fixes reported by Maciej Fijalkowski's review. .../net/ethernet/intel/i40e/i40e_ethtool.c | 3 - drivers/net/ethernet/intel/i40e/i40e_main.c | 15 +++-- drivers/net/ethernet/intel/i40e/i40e_txrx.c | 13 ++-- drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 - drivers/net/ethernet/intel/i40e/i40e_xsk.c | 67 ++++++++++++++++--- drivers/net/ethernet/intel/i40e/i40e_xsk.h | 2 +- 6 files changed, 73 insertions(+), 28 deletions(-)