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

[Jason Gunthorpe <jgg@nvidia.com>]

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