Re: [PATCH rdma-next 8/9] RDMA: Globally allocate and release QP memory

[Leon Romanovsky]

On Wed, Jul 21, 2021 at 02:05:34PM -0400, Dennis Dalessandro wrote:
> On 7/20/21 4:35 AM, Leon Romanovsky wrote:
> > On Mon, Jul 19, 2021 at 04:42:11PM +0300, Gal Pressman wrote:
> >> On 18/07/2021 15:00, Leon Romanovsky wrote:
> >>> From: Leon Romanovsky <leonro@xxxxxxxxxx>
> >>>
> >>> Convert QP object to follow IB/core general allocation scheme.
> >>> That change allows us to make sure that restrack properly kref
> >>> the memory.
> >>>
> >>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> >>
> >> EFA and core parts look good to me.
> >> Reviewed-by: Gal Pressman <galpress@xxxxxxxxxx>
> >> Tested-by: Gal Pressman <galpress@xxxxxxxxxx>
> 
> Leon, I pulled your tree and tested, things look good so far.
> 
> For rdmavt and core:
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>
> Tested-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>

Thanks

> 
> 
> > Thanks a lot.
> > 
> >>
> >>> +static inline void *rdma_zalloc_obj(struct ib_device *dev, size_t size,
> >>> +				    gfp_t gfp, bool is_numa_aware)
> >>> +{
> >>> +	if (is_numa_aware && dev->ops.get_numa_node)
> >>
> >> Honestly I think it's better to return an error if a numa aware allocation is
> >> requested and get_numa_node is not provided.
> > 
> > We don't want any driver to use and implement ".get_numa_node()" callback.
> > 
> > Initially, I thought about adding WARN_ON(driver_id != HFI && .get_numa_node)
> > to the device.c, but decided to stay with comment in ib_verbs.h only.
> 
> Maybe you could update that comment and add that it's for performance? This way
> its clear we are different for a reason. I'd be fine adding a WARN_ON_ONCE like
> you mention here. I don't think we need to fail the call but drawing attention
> to it would not necessarily be a bad thing. Either way, RB/TB for me stands.

The thing is that performance gain is achieved in very specific artificial
use case, while we can think about other scenario where such allocation will
give different results.

For the performance test, Mike forced to have QP and device to be in different
NUMA nodes, while in reality such situation is possible only if memory on the
local node is depleted and it is better to have working system instead of current
situation where node request will fail.

I don't want to give wrong impression that numa node allocations are
better for performance.

Thanks

> 
> -Denny
> 
>