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

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

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

On Sun, Mar 19, 2017 at 08:06:27PM +0530, Devesh Sharma wrote:
> 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.

Our past experience shows that developers tend to lost
interest and disappear once they submitted their code.

I'm not saying that you are one of such developers, but would be happy
to see fixed code before actually merging.

Thanks

>
> 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

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