Re: [PATCH 8/8] IB/srp: Drain the send queue before destroying a QP

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2274 bytes --]

On Fri, Feb 10, 2017 at 03:56:11PM -0800, Bart Van Assche wrote:
> A quote from the IB spec:
>
> However, if the Consumer does not wait for the Affiliated Asynchronous
> Last WQE Reached Event, then WQE and Data Segment leakage may occur.
> Therefore, it is good programming practice to tear down a QP that is
> associated with an SRQ by using the following process:
> * Put the QP in the Error State;
> * wait for the Affiliated Asynchronous Last WQE Reached Event;
> * either:
>   * drain the CQ by invoking the Poll CQ verb and either wait for CQ
>     to be empty or the number of Poll CQ operations has exceeded CQ
>     capacity size; or
>   * post another WR that completes on the same CQ and wait for this WR to return as a WC;
> * and then invoke a Destroy QP or Reset QP.
>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Cc: Israel Rukshin <israelr-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 2f85255d2aca..b50733910f7e 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -471,9 +471,13 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
>   * completion handler can access the queue pair while it is
>   * being destroyed.
>   */
> -static void srp_destroy_qp(struct ib_qp *qp)
> +static void srp_destroy_qp(struct srp_rdma_ch *ch, struct ib_qp *qp)
>  {
> -	ib_drain_rq(qp);
> +	spin_lock_irq(&ch->lock);
> +	ib_process_cq_direct(ch->send_cq, -1);

I see that you are already using "-1" in your code, but the comments in the
ib_process_cq_direct states that no new code should use "-1".

 61  * Note: for compatibility reasons -1 can be passed in %budget for unlimited
 62  * polling.  Do not use this feature in new code, it will be removed soon.
 63  */
 64 int ib_process_cq_direct(struct ib_cq *cq, int budget)

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]