Re: [Intel-wired-lan] [PATCH net v5] i40e: Fix DMA mappings leak

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

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_ */
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan