Message-ID: <983802a6-0fa2-e181-832e-13a2d5f0fa82@cornelisnetworks.com>
Date:   Thu, 20 May 2021 18:02:09 -0400
From:   Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Leon Romanovsky <leon@...nel.org>,
        "Marciniszyn, Mike" <mike.marciniszyn@...nelisnetworks.com>,
        Doug Ledford <dledford@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH rdma-next] RDMA/rdmavt: Decouple QP and SGE lists
 allocations

On 5/19/21 4:26 PM, Jason Gunthorpe wrote:
> On Wed, May 19, 2021 at 03:49:31PM -0400, Dennis Dalessandro wrote:
>> On 5/19/21 2:29 PM, Jason Gunthorpe wrote:
>>> On Wed, May 19, 2021 at 07:56:32AM -0400, Dennis Dalessandro wrote:
>>>
>>>> Perhaps the code can be enhanced to move more stuff into the driver's own
>>>> structs as Jason points out, but that should happen first. For now I still
>>>> don't understand why the core can't optionally make the allocation per node.
>>>
>>> Because I think it is wrong in the general case to assign all
>>> allocations to a single node?
>>
>> If by general case you mean for all drivers, sure, totally agree. We aren't
>> talking about all drivers though, just the particular case of rdmavt.
> 
> I think it is wrong for rdmavt too and your benchmarks have focused on
> a specific case with process/thread affinities that can actually
> benefit from it.
> 
> I don't want to encourage other drivers to do the same thing.

I would imagine they would get the same push back we are getting here. I 
don't think this would encourage anyone honestly.

> The correct thing to do today in 2021 is to use the standard NUMA
> memory policy on already node-affine threads. The memory policy goes
> into the kernel and normal non-_node allocations will obey it. When
> combined with an appropriate node-affine HCA this will work as you are
> expecting right now.

So we shouldn't see any issue in the normal case is what you are saying. 
I'd like to believe that, proving it is not easy though.

> However you can't do anything like that while the kernel has the _node
> annotations, that overrides the NUMA memory policy and breaks the
> policy system!

Does our driver doing this break the entire system? I'm not sure how 
that's possible. Is there an effort to get rid of these per node 
allocations so ultimately we won't have a choice at some point?

> The *only* reason to override the node behavior in the kernel is if
> the kernel knows with high certainty that allocations are only going
> to be touched by certain CPUs, such as because it knows that the
> allocation is substantially for use in a CPU pinned irq/workqeueue or
> accessed via DMA from a node affine DMA device.
> 
> None of these seem true for the QP struct.

Will let Mike M respond about that. He's got a much better handle on the 
implications.

> Especially since for RDMA all of the above is highly situational. The
> IRQ/WQ processing anything in RDMA should be tied to the comp_vector,
> so without knowing that information you simply can't do anything
> correct at allocation time.

I don't think that's true for our case. The comp_vector may in some 
cases be the right thing to dictate where memory should be, in our case 
I don't think that's true all the time.

> The idea of allocating every to the HW's node is simply not correct
> design. I will grant you it may have made sense ages ago before the
> NUMA stuff was more completed, but today it does not and you'd be
> better to remove it all and use memory policy properly than insist we
> keep it around forever.

Not insisting anything. If the trend is to remove these sort of 
allocations and other drivers are no longer doing this "not correct 
design" we are certainly open to change. We just want to understand the 
impact first rather than being strong armed into accepting a performance 
regression just so Leon can refactor some code.

-Denny

Powered by blists - more mailing lists