Re: [PATCH rdma-next] RDMA/rdmavt: Decouple QP and SGE lists allocations

[Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>]

On 5/25/21 9:13 AM, Jason Gunthorpe wrote:
> On Thu, May 20, 2021 at 06:02:09PM -0400, Dennis Dalessandro wrote:
> 
>>> 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.
> 
> Then we are back to making infrastructure that is only useful for one,
> arguably wrong, driver.

That's just it, you argue that it's wrong. We don't agree that it's 
wrong. In fact you said previously:

"
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.
"

Well, that's pretty much why we are doing this.

>>> 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.
> 
> Well, I said you have to setup the userspace properly, I'm not sure it
> just works out of the box.

I'm going to go out on a limb and assume it will not just work out of 
the box.

>>> 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.
> 
> It breaks your driver part of it, and if we lift it to the core code
> then it breaks all drivers, so it is a hard no-go.
> 
>> Is there an effort to get rid of these per node allocations so
>> ultimately we won't have a choice at some point?
> 
> Unlikely, subtle stuff like this will just be left broken in drivers
> nobody cares about..

If it's not that big of a deal then what's the problem? Again, you keep 
saying it's broken. I'm still not seeing a compelling reason that tells 
me it is in fact broken. This is the way we get best performance which 
for the RDMA subsystem should pretty much trump everything except security.

All this being said, philosophical and theoretical arguments aren't 
going to get us anywhere productive. Things could certainly be different 
performance wise half a decade later after the code originally went in.

We are already mid 5.13 cycle. So the earliest this could be queued up 
to go in is 5.14. Can this wait one more cycle? If we can't get it 
tested/proven to make a difference mid 5.14, we will drop the objection 
and Leon's patch can go ahead in for 5.15. Fair compromise?

-Denny