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
