Re: [Qemu-devel] [PATCH v2 3/4] hw/rdma: Modify create/destroy QP to support SRQ
> >>
> >> @@ -525,16 +539,21 @@ static int destroy_qp(PVRDMADev *dev, union
> >> pvrdma_cmd_req *req,
> >> struct pvrdma_cmd_destroy_qp *cmd = >destroy_qp;
> >> RdmaRmQP *qp;
> >> PvrdmaRing *ring;
> >> +uint8_t is_srq = 0;
> >>
> >> qp = rdma_rm_get_qp(>rdma_dev_res, cmd->qp_handle);
> >> if (!qp) {
> >> return -EINVAL;
> >> }
> >>
> >> +if (qp->is_srq) {
> >> +is_srq = 1;
> >> +}
> >> +
> >
> > [1]
> >
> >> rdma_rm_dealloc_qp(>rdma_dev_res, cmd->qp_handle);
> >
> > [2]
> >
> >>
> >> ring = (PvrdmaRing *)qp->opaque;
> >
> > [3]
> >
> >> -destroy_qp_rings(ring);
> >> +destroy_qp_rings(ring, is_srq);
> >
> > Better move the call to rdma_rm_dealloc_qp ([2]) to here and get rid of the
> > block in [1].
> >
> > In any case, the code in [3] looks like a bug to me (an existing bug), i.e.
> > qp pointer cannot be trusted after call to rdma_rm_dealloc_qp (use after
> > free).
> > What do you think?
>
> You are right, I'll rearrange the code in v3.
Thanks just please add note for that in the commit message as you are
fixing an issue not related to SRQ.
>
> >
> >>
> >> return 0;
> >> }
> >> --
> >> 2.20.1
> >>
> >>
>
Re: [Qemu-devel] [PATCH v2 3/4] hw/rdma: Modify create/destroy QP to support SRQ
On 3/27/19 5:54 PM, Yuval Shaia wrote:
> On Tue, Mar 26, 2019 at 02:54:32PM +0200, Kamal Heib wrote:
>> Modify create/destroy QP to support shared receive queue.
>>
>> Signed-off-by: Kamal Heib
>> ---
>> hw/rdma/rdma_backend.c | 9 --
>> hw/rdma/rdma_backend.h | 6 ++--
>> hw/rdma/rdma_rm.c| 23 +--
>> hw/rdma/rdma_rm.h| 3 +-
>> hw/rdma/rdma_rm_defs.h | 1 +
>> hw/rdma/vmw/pvrdma_cmd.c | 61 ++--
>> 6 files changed, 72 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
>> index 54419c8c58dd..8f1349c64dda 100644
>> --- a/hw/rdma/rdma_backend.c
>> +++ b/hw/rdma/rdma_backend.c
>> @@ -794,9 +794,9 @@ void rdma_backend_destroy_cq(RdmaBackendCQ *cq)
>>
>> int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
>> RdmaBackendPD *pd, RdmaBackendCQ *scq,
>> - RdmaBackendCQ *rcq, uint32_t max_send_wr,
>> - uint32_t max_recv_wr, uint32_t max_send_sge,
>> - uint32_t max_recv_sge)
>> + RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
>> + uint32_t max_send_wr, uint32_t max_recv_wr,
>> + uint32_t max_send_sge, uint32_t max_recv_sge)
>> {
>> struct ibv_qp_init_attr attr = {};
>>
>> @@ -824,6 +824,9 @@ int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t
>> qp_type,
>> attr.cap.max_recv_wr = max_recv_wr;
>> attr.cap.max_send_sge = max_send_sge;
>> attr.cap.max_recv_sge = max_recv_sge;
>> +if (srq) {
>> +attr.srq = srq->ibsrq;
>> +}
>>
>> qp->ibqp = ibv_create_qp(pd->ibpd, );
>> if (!qp->ibqp) {
>> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
>> index cad7956d98e8..7c1a19a2b5ff 100644
>> --- a/hw/rdma/rdma_backend.h
>> +++ b/hw/rdma/rdma_backend.h
>> @@ -89,9 +89,9 @@ void rdma_backend_poll_cq(RdmaDeviceResources
>> *rdma_dev_res, RdmaBackendCQ *cq);
>>
>> int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
>> RdmaBackendPD *pd, RdmaBackendCQ *scq,
>> - RdmaBackendCQ *rcq, uint32_t max_send_wr,
>> - uint32_t max_recv_wr, uint32_t max_send_sge,
>> - uint32_t max_recv_sge);
>> + RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
>> + uint32_t max_send_wr, uint32_t max_recv_wr,
>> + uint32_t max_send_sge, uint32_t max_recv_sge);
>> int rdma_backend_qp_state_init(RdmaBackendDev *backend_dev, RdmaBackendQP
>> *qp,
>> uint8_t qp_type, uint32_t qkey);
>> int rdma_backend_qp_state_rtr(RdmaBackendDev *backend_dev, RdmaBackendQP
>> *qp,
>> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
>> index bc5873cb4c14..90870ee0e15e 100644
>> --- a/hw/rdma/rdma_rm.c
>> +++ b/hw/rdma/rdma_rm.c
>> @@ -384,12 +384,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
>> uint32_t pd_handle,
>> uint8_t qp_type, uint32_t max_send_wr,
>> uint32_t max_send_sge, uint32_t send_cq_handle,
>> uint32_t max_recv_wr, uint32_t max_recv_sge,
>> - uint32_t recv_cq_handle, void *opaque, uint32_t *qpn)
>> + uint32_t recv_cq_handle, void *opaque, uint32_t *qpn,
>> + uint8_t is_srq, uint32_t srq_handle)
>> {
>> int rc;
>> RdmaRmQP *qp;
>> RdmaRmCQ *scq, *rcq;
>> RdmaRmPD *pd;
>> +RdmaRmSRQ *srq = NULL;
>> uint32_t rm_qpn;
>>
>> pd = rdma_rm_get_pd(dev_res, pd_handle);
>> @@ -406,6 +408,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
>> uint32_t pd_handle,
>> return -EINVAL;
>> }
>>
>> +if (is_srq) {
>> +srq = rdma_rm_get_srq(dev_res, srq_handle);
>> +if (!srq) {
>> +rdma_error_report("Invalid srqn %d", srq_handle);
>> +return -EINVAL;
>> +}
>> +}
>> +
>
> [1]
>
>> if (qp_type == IBV_QPT_GSI) {
>> scq->notify = CNT_SET;
>> rcq->notify = CNT_SET;
>> @@ -422,10 +432,17 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
>> uint32_t pd_handle,
>> qp->send_cq_handle = send_cq_handle;
>> qp->recv_cq_handle = recv_cq_handle;
>> qp->opaque = opaque;
>> +if (is_srq) {
>> +qp->is_srq = is_srq;
>> +srq->recv_cq_handle = recv_cq_handle;
>> +}
>
> Does it make sense to join this section with [1]?
I think that you are right, I'll fix it in v3.
>
>>
>> rc = rdma_backend_create_qp(>backend_qp, qp_type, >backend_pd,
>> ->backend_cq, >backend_cq,
>> max_send_wr,
>> -max_recv_wr, max_send_sge, max_recv_sge);
>> +>backend_cq, >backend_cq,
>> +
Re: [Qemu-devel] [PATCH v2 3/4] hw/rdma: Modify create/destroy QP to support SRQ
On Tue, Mar 26, 2019 at 02:54:32PM +0200, Kamal Heib wrote:
> Modify create/destroy QP to support shared receive queue.
>
> Signed-off-by: Kamal Heib
> ---
> hw/rdma/rdma_backend.c | 9 --
> hw/rdma/rdma_backend.h | 6 ++--
> hw/rdma/rdma_rm.c| 23 +--
> hw/rdma/rdma_rm.h| 3 +-
> hw/rdma/rdma_rm_defs.h | 1 +
> hw/rdma/vmw/pvrdma_cmd.c | 61 ++--
> 6 files changed, 72 insertions(+), 31 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index 54419c8c58dd..8f1349c64dda 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -794,9 +794,9 @@ void rdma_backend_destroy_cq(RdmaBackendCQ *cq)
>
> int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
> RdmaBackendPD *pd, RdmaBackendCQ *scq,
> - RdmaBackendCQ *rcq, uint32_t max_send_wr,
> - uint32_t max_recv_wr, uint32_t max_send_sge,
> - uint32_t max_recv_sge)
> + RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
> + uint32_t max_send_wr, uint32_t max_recv_wr,
> + uint32_t max_send_sge, uint32_t max_recv_sge)
> {
> struct ibv_qp_init_attr attr = {};
>
> @@ -824,6 +824,9 @@ int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t
> qp_type,
> attr.cap.max_recv_wr = max_recv_wr;
> attr.cap.max_send_sge = max_send_sge;
> attr.cap.max_recv_sge = max_recv_sge;
> +if (srq) {
> +attr.srq = srq->ibsrq;
> +}
>
> qp->ibqp = ibv_create_qp(pd->ibpd, );
> if (!qp->ibqp) {
> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> index cad7956d98e8..7c1a19a2b5ff 100644
> --- a/hw/rdma/rdma_backend.h
> +++ b/hw/rdma/rdma_backend.h
> @@ -89,9 +89,9 @@ void rdma_backend_poll_cq(RdmaDeviceResources
> *rdma_dev_res, RdmaBackendCQ *cq);
>
> int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
> RdmaBackendPD *pd, RdmaBackendCQ *scq,
> - RdmaBackendCQ *rcq, uint32_t max_send_wr,
> - uint32_t max_recv_wr, uint32_t max_send_sge,
> - uint32_t max_recv_sge);
> + RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
> + uint32_t max_send_wr, uint32_t max_recv_wr,
> + uint32_t max_send_sge, uint32_t max_recv_sge);
> int rdma_backend_qp_state_init(RdmaBackendDev *backend_dev, RdmaBackendQP
> *qp,
> uint8_t qp_type, uint32_t qkey);
> int rdma_backend_qp_state_rtr(RdmaBackendDev *backend_dev, RdmaBackendQP *qp,
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index bc5873cb4c14..90870ee0e15e 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -384,12 +384,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
> uint32_t pd_handle,
> uint8_t qp_type, uint32_t max_send_wr,
> uint32_t max_send_sge, uint32_t send_cq_handle,
> uint32_t max_recv_wr, uint32_t max_recv_sge,
> - uint32_t recv_cq_handle, void *opaque, uint32_t *qpn)
> + uint32_t recv_cq_handle, void *opaque, uint32_t *qpn,
> + uint8_t is_srq, uint32_t srq_handle)
> {
> int rc;
> RdmaRmQP *qp;
> RdmaRmCQ *scq, *rcq;
> RdmaRmPD *pd;
> +RdmaRmSRQ *srq = NULL;
> uint32_t rm_qpn;
>
> pd = rdma_rm_get_pd(dev_res, pd_handle);
> @@ -406,6 +408,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
> uint32_t pd_handle,
> return -EINVAL;
> }
>
> +if (is_srq) {
> +srq = rdma_rm_get_srq(dev_res, srq_handle);
> +if (!srq) {
> +rdma_error_report("Invalid srqn %d", srq_handle);
> +return -EINVAL;
> +}
> +}
> +
[1]
> if (qp_type == IBV_QPT_GSI) {
> scq->notify = CNT_SET;
> rcq->notify = CNT_SET;
> @@ -422,10 +432,17 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
> uint32_t pd_handle,
> qp->send_cq_handle = send_cq_handle;
> qp->recv_cq_handle = recv_cq_handle;
> qp->opaque = opaque;
> +if (is_srq) {
> +qp->is_srq = is_srq;
> +srq->recv_cq_handle = recv_cq_handle;
> +}
Does it make sense to join this section with [1]?
>
> rc = rdma_backend_create_qp(>backend_qp, qp_type, >backend_pd,
> ->backend_cq, >backend_cq,
> max_send_wr,
> -max_recv_wr, max_send_sge, max_recv_sge);
> +>backend_cq, >backend_cq,
> +is_srq ? >backend_srq : NULL,
> +max_send_wr, max_recv_wr, max_send_sge,
> +max_recv_sge);
> +
> if (rc) {
> rc = -EIO;
>
[Qemu-devel] [PATCH v2 3/4] hw/rdma: Modify create/destroy QP to support SRQ
Modify create/destroy QP to support shared receive queue.
Signed-off-by: Kamal Heib
---
hw/rdma/rdma_backend.c | 9 --
hw/rdma/rdma_backend.h | 6 ++--
hw/rdma/rdma_rm.c| 23 +--
hw/rdma/rdma_rm.h| 3 +-
hw/rdma/rdma_rm_defs.h | 1 +
hw/rdma/vmw/pvrdma_cmd.c | 61 ++--
6 files changed, 72 insertions(+), 31 deletions(-)
diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 54419c8c58dd..8f1349c64dda 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -794,9 +794,9 @@ void rdma_backend_destroy_cq(RdmaBackendCQ *cq)
int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
RdmaBackendPD *pd, RdmaBackendCQ *scq,
- RdmaBackendCQ *rcq, uint32_t max_send_wr,
- uint32_t max_recv_wr, uint32_t max_send_sge,
- uint32_t max_recv_sge)
+ RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
+ uint32_t max_send_wr, uint32_t max_recv_wr,
+ uint32_t max_send_sge, uint32_t max_recv_sge)
{
struct ibv_qp_init_attr attr = {};
@@ -824,6 +824,9 @@ int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t
qp_type,
attr.cap.max_recv_wr = max_recv_wr;
attr.cap.max_send_sge = max_send_sge;
attr.cap.max_recv_sge = max_recv_sge;
+if (srq) {
+attr.srq = srq->ibsrq;
+}
qp->ibqp = ibv_create_qp(pd->ibpd, );
if (!qp->ibqp) {
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index cad7956d98e8..7c1a19a2b5ff 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -89,9 +89,9 @@ void rdma_backend_poll_cq(RdmaDeviceResources *rdma_dev_res,
RdmaBackendCQ *cq);
int rdma_backend_create_qp(RdmaBackendQP *qp, uint8_t qp_type,
RdmaBackendPD *pd, RdmaBackendCQ *scq,
- RdmaBackendCQ *rcq, uint32_t max_send_wr,
- uint32_t max_recv_wr, uint32_t max_send_sge,
- uint32_t max_recv_sge);
+ RdmaBackendCQ *rcq, RdmaBackendSRQ *srq,
+ uint32_t max_send_wr, uint32_t max_recv_wr,
+ uint32_t max_send_sge, uint32_t max_recv_sge);
int rdma_backend_qp_state_init(RdmaBackendDev *backend_dev, RdmaBackendQP *qp,
uint8_t qp_type, uint32_t qkey);
int rdma_backend_qp_state_rtr(RdmaBackendDev *backend_dev, RdmaBackendQP *qp,
diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index bc5873cb4c14..90870ee0e15e 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -384,12 +384,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
uint32_t pd_handle,
uint8_t qp_type, uint32_t max_send_wr,
uint32_t max_send_sge, uint32_t send_cq_handle,
uint32_t max_recv_wr, uint32_t max_recv_sge,
- uint32_t recv_cq_handle, void *opaque, uint32_t *qpn)
+ uint32_t recv_cq_handle, void *opaque, uint32_t *qpn,
+ uint8_t is_srq, uint32_t srq_handle)
{
int rc;
RdmaRmQP *qp;
RdmaRmCQ *scq, *rcq;
RdmaRmPD *pd;
+RdmaRmSRQ *srq = NULL;
uint32_t rm_qpn;
pd = rdma_rm_get_pd(dev_res, pd_handle);
@@ -406,6 +408,14 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
uint32_t pd_handle,
return -EINVAL;
}
+if (is_srq) {
+srq = rdma_rm_get_srq(dev_res, srq_handle);
+if (!srq) {
+rdma_error_report("Invalid srqn %d", srq_handle);
+return -EINVAL;
+}
+}
+
if (qp_type == IBV_QPT_GSI) {
scq->notify = CNT_SET;
rcq->notify = CNT_SET;
@@ -422,10 +432,17 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res,
uint32_t pd_handle,
qp->send_cq_handle = send_cq_handle;
qp->recv_cq_handle = recv_cq_handle;
qp->opaque = opaque;
+if (is_srq) {
+qp->is_srq = is_srq;
+srq->recv_cq_handle = recv_cq_handle;
+}
rc = rdma_backend_create_qp(>backend_qp, qp_type, >backend_pd,
->backend_cq, >backend_cq,
max_send_wr,
-max_recv_wr, max_send_sge, max_recv_sge);
+>backend_cq, >backend_cq,
+is_srq ? >backend_srq : NULL,
+max_send_wr, max_recv_wr, max_send_sge,
+max_recv_sge);
+
if (rc) {
rc = -EIO;
goto out_dealloc_qp;
diff --git a/hw/rdma/rdma_rm.h b/hw/rdma/rdma_rm.h
index e88ab95e264b..e8639909cd34 100644
--- a/hw/rdma/rdma_rm.h
+++ b/hw/rdma/rdma_rm.h
@@ -53,7 +53,8 @@ int rdma_rm_alloc_qp(RdmaDeviceResources *dev_res, uint32_t
pd_handle,
uint8_t qp_type, uint32_t max_send_wr,
uint32_t max_send_sge,
