Re: [PATCH net-next 0/9] mlx4: order-0 allocations and page recycling

[Tariq Toukan]

On 08/02/2017 5:52 PM, Eric Dumazet wrote:
On Wed, 2017-02-08 at 11:02 +0200, Tariq Toukan wrote:
On 07/02/2017 5:50 PM, Tariq Toukan wrote:
Hi Eric,

Thanks for your series.

On 07/02/2017 5:02 AM, Eric Dumazet wrote:
As mentioned half a year ago, we better switch mlx4 driver to order-0
allocations and page recycling.

This reduces vulnerability surface thanks to better skb->truesize
tracking
and provides better performance in most cases.
The series makes significant change in the RX data-path, that requires
deeper checks, in addition to code review.
We applied your series and started running both our functional and
performance regression.
We will have results by tomorrow morning, and will analyze them during
the day. I'll update about that.
We hit a kernel panic when running traffic after configuring a large MTU
(9000).
I will take deeper look into this soon and will keep you updated.
Hmm... I saw a typo for XDP, but not for the non XDP path...

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c 
b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 
8f16ec8dfadd0f95646c498c14d53f7266a0..e572e175edfe0f7392b9833b5b3f867fd6db 
100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -132,7 +132,7 @@ static int mlx4_en_prepare_rx_desc(const struct 
mlx4_en_priv *priv,
                                         (index << priv->log_rx_info);
  
         if (ring->page_cache.index > 0) {
-               if (frags[0].page) {
+               if (!frags[0].page) {
                         ring->page_cache.index--;
                         frags[0].page = 
ring->page_cache.buf[ring->page_cache.index].page;
                         frags[0].dma  = 
ring->page_cache.buf[ring->page_cache.index].dma;


Yes.

Here it is:
Large MTU enlarges priv->log_rx_info.

1115         priv->log_rx_info = ROUNDUP_LOG2(i * sizeof(struct 
mlx4_en_rx_alloc));

In the free_frag function, only frag->page is cleared (dma and offset 
are not!), leaving non-zero garbage that is read later as a page field.

  90 static void mlx4_en_free_frag(const struct mlx4_en_priv *priv,
  91                               struct mlx4_en_rx_alloc *frag)
  92 {
  93         if (frag->page) {
  94                 dma_unmap_page(priv->ddev, frag->dma,
  95                                PAGE_SIZE, priv->dma_dir);
  96                 put_page(frag->page);
  97         }

Later, on line 82, we stay with a garbage page pointer.

  74 static int mlx4_en_alloc_frags(const struct mlx4_en_priv *priv,
  75                                struct mlx4_en_rx_desc *rx_desc,
  76                                struct mlx4_en_rx_alloc *frags,
  77                                gfp_t gfp)
  78 {
  79         int i;
  80
  81         for (i = 0; i < priv->num_frags; i++, frags++) {
  82                 if (!frags->page && mlx4_alloc_page(priv, frags, gfp))
  83                         return -ENOMEM;
  84                 rx_desc->data[i].addr = cpu_to_be64(frags->dma +
  85 frags->page_offset);
  86         }
  87         return 0;
  88 }

It can be fixed with this:

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c 
b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 6854a19087ed..d97ee69393f0 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -94,8 +94,8 @@ static void mlx4_en_free_frag(const struct 
mlx4_en_priv *priv,
                dma_unmap_page(priv->ddev, frag->dma,
                               PAGE_SIZE, priv->dma_dir);
                put_page(frag->page);
-               frag->page = NULL;
        }
+       memset(frag, 0, sizeof(*frag));
 }

 static void mlx4_en_init_rx_desc(const struct mlx4_en_priv *priv,


Regards,
Tariq Toukan.