Re: [rdma-core v3 5/9] libbnxt_re: Allow apps to poll for flushed completions

Devesh Sharma
2017-03-19T14:36:27Z

Hi Leon,

Will it okay if I revisit this change as a bug fix I need to validate
the entire scheme with the suggested change.

On Sun, Mar 19, 2017 at 1:42 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Mar 16, 2017 at 08:52:27PM +0530, Devesh Sharma wrote:
>> On Thu, Mar 16, 2017 at 12:50 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Wed, Mar 15, 2017 at 06:37:29AM -0400, Devesh Sharma wrote:
>> >> This patch adds support for reporting flush completions.
>> >> following is the overview of the algorithm used.
>> >>
>> >> Step-1: Poll a completion from h/w CQ.
>> >> Step-2: check the status, if it is error goto step3 else report
>> >> completion to user based on the con_idx reported.
>> >> Step-3: Report the completion with actual error to consumer, and
>> >> without bothering about the con_idx reported in the
>> >> completion do following:
>> >> 3a. Add this QP to the CQ flush list if it was not there
>> >> already. If this is req-error, add the QP to send-flush
>> >> list, else add it to recv-flush-list.
>> >> 3b. Change QP-soft-state to ERROR if it was not in error
>> >> already.
>> >>
>> >> Step-4: If next CQE is TERM CQE, extract this CQE. make sure this CQE
>> >> is not reported to the consumer. Do the following steps as
>> >> further processing:
>> >> 4a. Add this QP to both send-flush-list and recv-flush-list
>> >> if QP is absent from any of the flush lists.
>> >> 4b. Change QP-soft-state to ERROR if it was not in error
>> >> already.
>> >> Step5: Continue polling from both h/w CQ and flush-lists until
>> >> all the queues are empty.
>> >>
>> >> The QP is removed from the flush list during destroy-qp.
>> >>
>> >> Further, it adds network to host format conversion on
>> >> the received immediate data.
>> >>
>> >> This patch also takes care of Hardware specific requirement
>> >> to skip reporting h/w flush error CQEs to consumer but ring
>> >> the CQ-DB for them.
>> >>
>> >> v1->v2
>> >> -- Used ccan/list.h instead.
>> >>
>> >> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> >> Signed-off-by: Somnath Kotur <somnath.kotur-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> >> Signed-off-by: Selvin Xavier <selvin.xavier-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> >> Signed-off-by: Devesh Sharma <devesh.sharma-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> >> ---
>> >> providers/bnxt_re/flush.h | 85 ++++++++++++++++
>> >> providers/bnxt_re/main.c | 5 +
>> >> providers/bnxt_re/main.h | 6 ++
>> >> providers/bnxt_re/memory.h | 5 +
>> >> providers/bnxt_re/verbs.c | 243 ++++++++++++++++++++++++++++++++++++++++-----
>> >> 5 files changed, 320 insertions(+), 24 deletions(-)
>> >> create mode 100644 providers/bnxt_re/flush.h
>> >>
>> >> diff --git a/providers/bnxt_re/flush.h b/providers/bnxt_re/flush.h
>> >> new file mode 100644
>> >> index 0000000..a39ea71
>> >> --- /dev/null
>> >> +++ b/providers/bnxt_re/flush.h
>> >> @@ -0,0 +1,85 @@
>> >> +/*
>> >> + * Broadcom NetXtreme-E User Space RoCE driver
>> >> + *
>> >> + * Copyright (c) 2015-2017, Broadcom. All rights reserved. The term
>> >> + * Broadcom refers to Broadcom Limited and/or its subsidiaries.
>> >> + *
>> >> + * This software is available to you under a choice of one of two
>> >> + * licenses. You may choose to be licensed under the terms of the GNU
>> >> + * General Public License (GPL) Version 2, available from the file
>> >> + * COPYING in the main directory of this source tree, or the
>> >> + * BSD license below:
>> >> + *
>> >> + * Redistribution and use in source and binary forms, with or without
>> >> + * modification, are permitted provided that the following conditions
>> >> + * are met:
>> >> + *
>> >> + * 1. Redistributions of source code must retain the above copyright
>> >> + * notice, this list of conditions and the following disclaimer.
>> >> + * 2. Redistributions in binary form must reproduce the above copyright
>> >> + * notice, this list of conditions and the following disclaimer in
>> >> + * the documentation and/or other materials provided with the
>> >> + * distribution.
>> >> + *
>> >> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
>> >> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>> >> + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>> >> + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS
>> >> + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> >> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> >> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>> >> + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE
>> >> + * OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN
>> >> + * IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> >> + *
>> >> + * Description: A few wrappers for flush queue management
>> >> + */
>> >> +
>> >> +#ifndef __FLUSH_H__
>> >> +#define __FLUSH_H__
>> >> +
>> >> +#include <ccan/list.h>
>> >> +
>> >> +struct bnxt_re_fque_node {
>> >> + uint8_t valid;
>> >> + struct list_node list;
>> >> +};
>> >> +
>> >> +static inline void fque_init_node(struct bnxt_re_fque_node *node)
>> >> +{
>> >> + list_node_init(&node->list);
>> >> + node->valid = false;
>> >> +}
>> >> +
>> >> +static inline void fque_add_node_tail(struct list_head *head,
>> >> + struct bnxt_re_fque_node *new)
>> >> +{
>> >> + list_add_tail(head, &new->list);
>> >> + new->valid = true;
>> >> +}
>> >> +
>> >> +static inline void fque_del_node(struct bnxt_re_fque_node *entry)
>> >> +{
>> >> + entry->valid = false;
>> >> + list_del(&entry->list);
>> >> +}
>> >> +
>> >
>> > I don't see the point of these wrappers.
>>
>> Wanted to keep "entry->valid" as a property of list element rather
>> mixing it in the QP structure to keep things clean. Thus, could not
>> resist to keep those.
>
> It looks like list == NULL can do the trick.
>
> Thanks