Message-ID: <be92f9c6-c7a8-b013-9eb4-057129f0edfe@mellanox.com>
Date:   Thu, 3 Jan 2019 14:32:34 +0000
From:   Tariq Toukan <tariqt@...lanox.com>
To:     Stephen Warren <swarren@...dotorg.org>,
        Tariq Toukan <tariqt@...lanox.com>
CC:     Jason Gunthorpe <jgg@...pe.ca>,
        "xavier.huwei@...wei.com" <xavier.huwei@...wei.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Doug Ledford <dledford@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Stephen Warren <swarren@...dia.com>
Subject: Re: [PATCH V3] net/mlx4: Get rid of page operation after
 dma_alloc_coherent



On 1/2/2019 6:29 PM, Stephen Warren wrote:
> On 12/20/18 10:43 AM, Jason Gunthorpe wrote:
>> On Wed, Dec 19, 2018 at 11:20:31AM -0700, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@...dia.com>
>>>
>>> This patch solves a crash at the time of mlx4 driver unload or system
>>> shutdown. The crash occurs because dma_alloc_coherent() returns one
>>> value in mlx4_alloc_icm_coherent(), but a different value is passed to
>>> dma_free_coherent() in mlx4_free_icm_coherent(). In turn this is because
>>> when allocated, that pointer is passed to sg_set_buf() to record it,
>>> then when freed it is re-calculated by calling
>>> lowmem_page_address(sg_page()) which returns a different value. Solve
>>> this by recording the value that dma_alloc_coherent() returns, and
>>> passing this to dma_free_coherent().
>>>
>>> This patch is roughly equivalent to commit 378efe798ecf ("RDMA/hns: Get
>>> rid of page operation after dma_alloc_coherent"). That patch was
>>> described as:
>>>
>>>> In general, dma_alloc_coherent() returns a CPU virtual address and
>>>> a DMA address, and we have no guarantee that the underlying memory
>>>> even has an associated struct page at all.
>>>>
>>>> This patch gets rid of the page operation after dma_alloc_coherent,
>>>> and records the VA returned form dma_alloc_coherent in the struct
>>>> of hem in hns RoCE driver.
>>>
>>> However, this patch reworks the code to store all information about
>>> coherent chunks separately from the sg list, since using sg lists for
>>> them doesn't make sense. Hence, the structure of this patch is quite
>>> different compared to the hns patch.
>>>
>>> Based-on-code-from: Christoph Hellwig <hch@....de>
>>> Signed-off-by: Stephen Warren <swarren@...dia.com>
>>> ---
>>> v3:
>>> - Rework chunk data structure to store all data for coherent allocations
>>>    separately from the sg list. Code from Christoph Hellwig with 
>>> fixes by
>>>    me. Notes:
>>>    - chunk->coherent is an int not a bool since checkpatch complains 
>>> about
>>>      using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
>>
>> :( bool is much more readable and there is no performance concern in
>> this struct. I think checkpatch is overzealous here.
>>
>>>    - chunk->coherent is used rather than chunk->table->coherent since 
>>> the
>>>      table pointer isn't available when creating chunks. This duplicates
>>>      data, but simplifies the patch.
>>> v2:
>>> - Rework mlx4_table_find() to explicitly calculate the returned address
>>>    differently depending on wheter the table was allocated using
>>>    dma_alloc_coherent() or alloc_pages(), which in turn allows the
>>>    changes to mlx4_alloc_icm_pages() to be dropped.
>>> - Drop changes to mlx4_alloc/free_icm_pages. This path uses
>>>    pci_map_sg() which can re-write the sg list which in turn would cause
>>>    chunk->mem[] (the sg list) and chunk->buf[] to become inconsistent.
>>> - Enhance commit description.
>>>
>>> Note: I've tested this patch in a downstream 4.14 based kernel (using
>>> ibping, ib_read_bw, and ib_write_bw), but can't test it in mainline
>>> since my system isn't supported there yet. I have compile-tested it in
>>> mainline at least, for ARM64.
>>> ---
>>>   drivers/net/ethernet/mellanox/mlx4/icm.c | 92 ++++++++++++++----------
>>>   drivers/net/ethernet/mellanox/mlx4/icm.h | 22 +++++-
>>>   2 files changed, 75 insertions(+), 39 deletions(-)
>>
>> I think this needs an ack from the driver maintainers, and I gather
>> they are all on break for the next two weeks or so.
>>
>> Lets revisit in January?
>>
>> But at first glance it looks OK to me, though I would tidy the commit
>> message somewhat, your new leading paragraph fully explains the
>> problem, no need for the hns quote - and we can now see that the hns
>> should't have used a sgl either...
> 
> Tariq, I assume you're back from vacation now/soon. What do you think of 
> this patch, aside from the bool-vs-int issue that I plan to fix up soon. 
> Thanks.

Hi Stephen,

Thanks for your patch.
It looks good to me.

Tariq

Powered by blists - more mailing lists