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
