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

[Tariq Toukan <ttoukan.linux@gmail.com>]

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.