Re: [REPOST PATCH] drivers: net: mvpp2: attempt to refill rx before allocating skb

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

> on mvpp2_rx_refill() failure, the freshly allocated skb is freed,
> the rx error counter is incremented and the descriptor currently
> being processed is rearmed through mvpp2_bm_pool_put().
> 
> the logic is that the system is low on memory so it's not possible
> to allocate both a rx descriptor and an skb, so we might as well
> drop the skb and return the descriptor to the rx pool to avoid
> draining it (and preventing any future packet reception).

Hi Marios,

Can we just run the mvpp2_rx_refill() when the skb has been already
successfully sent to the networking stack? Doing so, if the skb allocation
fails, we will refill the in-flight rx descriptor (the one consumed by the
skb) to the descriptor ring, reducing the pressure on it.
What do you think?

Regards,
Lorenzo

> 
> the skb freeing is unfortunate, as build_skb() takes ownership
> of the 'data' buffer:
>  - build_skb() calls  __finalize_skb_around() which sets skb->head
>  and skb->data to point to 'data'
>  - dev_free_skb_any() may call skb_free_frag() on skb->head
> 
> thus, the final mvpp2_bm_pool_put() rearms a descriptor that was
> just freed.
> 
> call mvpp2_rx_refill() first, so there's no skb to free.
> 
> incidentally, doing rx refill prior to skb allocation is what is
> done in marvell's mvneta driver for armada 370 (mvneta_rx_hwbm() in
> mvneta.c)
> 
> Fixes: d6526926de739 ("net: mvpp2: fix memory leak in mvpp2_rx")
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 416a926a8281..e13055ec4483 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -4003,6 +4003,12 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  			metasize = xdp.data - xdp.data_meta;
>  		}
>  
> +		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
> +		if (err) {
> +			netdev_err(port->dev, "failed to refill BM pools\n");
> +			goto err_drop_frame;
> +		}
> +
>  		if (frag_size)
>  			skb = build_skb(data, frag_size);
>  		else
> @@ -4021,13 +4027,6 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
>  					 skb_hwtstamps(skb));
>  		}
>  
> -		err = mvpp2_rx_refill(port, bm_pool, pp, pool);
> -		if (err) {
> -			netdev_err(port->dev, "failed to refill BM pools\n");
> -			dev_kfree_skb_any(skb);
> -			goto err_drop_frame;
> -		}
> -
>  		if (pp)
>  			skb_mark_for_recycle(skb);
>  		else
> -- 
> 2.49.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]