Skip to content

mlx5: ignore QP max_recv_wr when SRQ is used #1624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FujiZ
Copy link
Contributor

@FujiZ FujiZ commented Jul 8, 2025

According to the manual, max_recv_wr and max_recv_sge are ignored by ibv_create_qp if the QP is to be associated with an SRQ. Fix the bug where ibv_create_qp may return EINVAL when attr->cap.max_recv_wr > cap.max_recv_wr even if the QP is to be associated with SRQ.

The code snippet to reproduce this issue:

#include <stdio.h>
#include <stdlib.h>
#include <infiniband/verbs.h>

int main() {
    struct ibv_device **dev_list;
    struct ibv_context *context;
    struct ibv_pd *pd;
    struct ibv_srq_init_attr srq_init_attr = {0};
    struct ibv_srq *srq;
    struct ibv_qp_init_attr qp_init_attr = {0};
    struct ibv_qp *qp;

    dev_list = ibv_get_device_list(NULL);
    if (!dev_list) {
        perror("Failed to get IB devices list");
        return -1;
    }

    context = ibv_open_device(dev_list[0]);
    if (!context) {
        perror("Failed to open IB device");
        return -1;
    }

    pd = ibv_alloc_pd(context);
    if (!pd) {
        perror("Failed to allocate PD");
        return -1;
    }

    srq_init_attr.attr.max_wr  = 1;
    srq_init_attr.attr.max_sge = 1;
    srq = ibv_create_srq(pd, &srq_init_attr);
    if (!srq) {
        perror("Failed to create SRQ");
        return -1;
    }
    printf("SRQ created successfully\n");

    struct ibv_cq* cq = ibv_create_cq(context, 10, NULL, NULL, 0);
    if (!cq) {
        perror("Failed to create CQ");
        return -1;
    }

    qp_init_attr.qp_type    = IBV_QPT_RC;
    qp_init_attr.srq        = srq;
    qp_init_attr.send_cq    = cq;
    qp_init_attr.recv_cq    = cq;
    qp_init_attr.cap.max_send_wr  = 1;
    // fill in random value for max_recv_wr, which should be ignored
    qp_init_attr.cap.max_recv_wr  = UINT32_MAX;
    qp_init_attr.cap.max_send_sge = 1;
    qp_init_attr.cap.max_recv_sge = 0;

    qp = ibv_create_qp(pd, &qp_init_attr);
    if (!qp) {
        perror("Failed to create QP");
        return -1;
    }
    printf("QP created using SRQ successfully\n");
    return 0;
}

According to the manual, max_recv_wr and max_recv_sge are ignored by
`ibv_create_qp` if the QP is to be associated with an SRQ. Fix the bug
where `ibv_create_qp` may fail due to uninitialized `max_recv_wr`.

Signed-off-by: ZHOU Huaping <[email protected]>
@FujiZ FujiZ force-pushed the zhp/ignore-srq-recv-wr branch from 34f065f to d5e4173 Compare July 8, 2025 09:26
@FujiZ FujiZ marked this pull request as ready for review July 8, 2025 09:28
@@ -1770,6 +1770,12 @@ static int mlx5_get_max_recv_wr(struct mlx5_context *ctx,
struct mlx5dv_qp_init_attr *mlx5_qp_attr,
uint32_t *max_recv_wr)
{
/* Ignore max_recv_wr when SRQ is used. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the issue that you hit without that change ? not clear from the code and the commit log.

What are the values of the below in your case ?

ctx->max_recv_wr
attr->cap.max_recv_wr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my case attr->cap.max_recv_wr is way greater than ctx->max_recv_wr because it contains random bytes on stack. However, since the manual notes that "The attributes max_recv_wr and max_recv_sge are ignored by ibv_create_qp() if the QP is to be associated with an SRQ" (https://man7.org/linux/man-pages/man3/ibv_create_qp.3.html), I believe that any value of attr->cap.max_recv_wr is acceptable when QP is associated with SRQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change I will get EINVAL from ibv_create_qp() if attr->cap.max_recv_wr > cap.max_recv_wr even if the QP is to be associated with SRQ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked around at other drivers, is that case handled at all ?

Why should an application use SRQ and pass attr->cap.max_recv_wr which is different than 0 at all ?

If the issue is really important to handle, we may consider setting attr->cap.max_recv_wr to 0 in the verbs layer to let it work for all drivers around.

Please check and let's get other feedback here.

Copy link
Contributor Author

@FujiZ FujiZ Jul 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked around at other drivers, is that case handled at all ?

I'm not sure if other drivers handle this issue correctly, since I only have mellanox NIC at hand.

Why should an application use SRQ and pass attr->cap.max_recv_wr which is different than 0 at all ?

Because as I mentioned before, the manual said that "The attributes max_recv_wr and max_recv_sge are ignored by ibv_create_qp() if the QP is to be associated with an SRQ" (https://man7.org/linux/man-pages/man3/ibv_create_qp.3.html), which means that assigning different values to max_recv_wr should produce the same result when SRQ is used. Besides, the manual didn't mention that "The attributes max_recv_wr and max_recv_sge should be set to 0 if the QP is to be associated with an SRQ", so it is valid for an application to use SRQ and pass attr->cap.max_recv_wr which is different than 0. The value of attr->cap.max_recv_wr may come from uninitialized stack variables, since the application may construct the ibv_qp_init_attr on stack and not explicitly set the max_recv_wr when using SRQ.

If the issue is really important to handle, we may consider setting attr->cap.max_recv_wr to 0 in the verbs layer to let it work for all drivers around.

Yeah I think that is viable choice, too.

@rleon
Copy link
Member

rleon commented Jul 24, 2025

do we have resolution here?

@yishaih
Copy link
Member

yishaih commented Jul 24, 2025

I would expect another general solution that will fit all drivers and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants