On 9/23/2022 10:24 PM, Mark Zhang wrote:
External email: Use caution opening links or attachments
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).
+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.
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?
Sorry what I mean is resp_pr_data is used by netlink while mad->data is
used by post_send_mad(), so if we want to always use resp_pr_data then
in post_send_mad() we need to do malloc, copy and transfer. Besides if
malloc failures then we still need to use mad->data, unless we always
use stack no matter how many PRs there are.
Use malloc instead of stack for resp_pr_data and multiple-PR unpack is
because sizeof(sa_path_rec)=72B, now we supports 3 and there might be
more in future..
