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??
> static void cma_query_handler(int status, struct sa_path_rec *path_rec,
> - void *context)
> + int num_prs, void *context)
This param should be "unsigned int num_prs"
> {
> struct cma_work *work = context;
> struct rdma_route *route;
> + int i;
>
> route = &work->id->id.route;
>
> - if (!status) {
> - route->num_pri_alt_paths = 1;
> - *route->path_rec = *path_rec;
> - } else {
> - work->old_state = RDMA_CM_ROUTE_QUERY;
> - work->new_state = RDMA_CM_ADDR_RESOLVED;
> - work->event.event = RDMA_CM_EVENT_ROUTE_ERROR;
> - work->event.status = status;
> - pr_debug_ratelimited("RDMA CM: ROUTE_ERROR: failed to query path. status %d\n",
> - status);
> + if (status)
> + goto fail;
> +
> + for (i = 0; i < num_prs; i++) {
> + if (!path_rec[i].flags || (path_rec[i].flags & IB_PATH_GMP))
> + *route->path_rec = path_rec[i];
> + else if (path_rec[i].flags & IB_PATH_INBOUND)
> + route_set_path_rec_inbound(work, &path_rec[i]);
> + else if (path_rec[i].flags & IB_PATH_OUTBOUND)
> + route_set_path_rec_outbound(work, &path_rec[i]);
> + }
> + if (!route->path_rec) {
Why is this needed? The for loop above will have already oops'd.
> +/**
> + * ib_sa_pr_callback_multiple() - Parse path records then do callback.
> + *
> + * In a multiple-PR case the PRs are saved in "query->resp_pr_data"
> + * (instead of"mad->data") and with "ib_path_rec_data" structure format,
> + * so that rec->flags can be set to indicate the type of PR.
> + * This is valid only in IB fabric.
> + */
> +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.
> diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
> index 81916039ee24..cdc7cafab572 100644
> --- a/include/rdma/rdma_cm.h
> +++ b/include/rdma/rdma_cm.h
> @@ -49,9 +49,15 @@ struct rdma_addr {
> struct rdma_dev_addr dev_addr;
> };
>
> +#define RDMA_PRIMARY_PATH_MAX_REC_NUM 3
This is a strange place for this define, it should be in sa_query.c?
Jason
