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 10:24:35PM +0800, Mark Zhang wrote:
> On 9/23/2022 9:13 PM, Jason Gunthorpe wrote:
> > 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?
> 
> Because inbound/outbound PRs are optional, so even they are provided they
> can still be ignored if cma is not able to set them (e.g. memory allocation
> failure in this case).

This isn't how we do things, the netlink operation had a failure, the
failure should propogate. If we've run out of memory the CM connection
is going to blow up anyhow very quickly.

> > We should always be able to point resp_pr_data to some kind of
> > storage, even if it is stack storage.
> 
> The idea is:
> - Single PR: PR in mad->data; Used by both netlink and
>   ib_post_send_mad();
> - Multiple PRs: PRs in resp_pr_data, with "ib_path_rec_data" structure
>   format; Currently used by netlink only.
> 
> So if we want to always use resp_pr_data then in single-PR case we need to
> copy mad->data to resp_pr_data in both netlink and ib_post_send_mad(), and
> turn it into "ib_path_rec_data" structure format. This adds complexity for
> single-PR, which should be most of situations?

You don't copy it, you unpack it. We already have to unpack it, just
to a (large!) stack var - unpack it to resp_pr_data instead.

Jason