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;
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"
Ack
{
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.
Right, this "if" is no needed. We don't need to check if route->path_rec
is valid in this function because it is allocated in cma_resolve_ib_route()
+/**
+ * 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.
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.
>> 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?
That's because path_rec, path_rec_inbound and path_rec_outbound are
defined here, but yes it is only used in sa_query.c, so maybe better
move it to there.
Thanks Jason.
