Re: [PATCH rdma-next 2/4] RDMA/cma: Multiple path records support with netlink channel

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Sep 23, 2022 at 09:40:22AM +0800, Mark Zhang wrote:
> On 9/22/2022 9:58 PM, Jason Gunthorpe wrote:
> > On Thu, Sep 08, 2022 at 01:09:01PM +0300, Leon Romanovsky wrote:
> > 
> > > +static void route_set_path_rec_inbound(struct cma_work *work,
> > > +				       struct sa_path_rec *path_rec)
> > > +{
> > > +	struct rdma_route *route = &work->id->id.route;
> > > +
> > > +	if (!route->path_rec_inbound) {
> > > +		route->path_rec_inbound =
> > > +			kzalloc(sizeof(*route->path_rec_inbound), GFP_KERNEL);
> > > +		if (!route->path_rec_inbound)
> > > +			return;
> > > +	}
> > > +
> > > +	*route->path_rec_inbound = *path_rec;
> > > +}
> > 
> > We are just ignoring these memory allocation failures??
> > 
> Inside "if" statement if kzalloc fails here then we don't set
> route->path_rec_inbound or outbound;

But why don't we propogate a ENOMEM failure code?
> > > +static void ib_sa_pr_callback_multiple(struct ib_sa_path_query *query,
> > > +				       int status, int num_prs,
> > > +				       struct ib_path_rec_data *rec_data)
> > > +{
> > > +	struct sa_path_rec *rec;
> > > +	int i;
> > > +
> > > +	rec = kvcalloc(num_prs, sizeof(*rec), GFP_KERNEL);
> > > +	if (!rec) {
> > > +		query->callback(-ENOMEM, NULL, 0, query->context);
> > > +		return;
> > > +	}
> > 
> > This all seems really wild, why are we allocating memory so many times
> > on this path? Have ib_nl_process_good_resolve_rsp unpack the mad
> > instead of storing the raw format
> > 
> > It would also be good to make resp_pr_data always valid so all these
> > special paths don't need to exist.
> 
> The ib_sa_pr_callback_single() uses stack variable "rec" but
> ib_sa_pr_callback_multiple() uses malloc because there are multiple PRs.
> 
> ib_sa_path_rec_callback is also used by ib_post_send_mad(), which always
> have single PR and saved in mad->data, so always set resp_pr_data in netlink
> case is not enough.

We should always be able to point resp_pr_data to some kind of
storage, even if it is stack storage.

Jason