From patchwork Wed Jun 20 13:21:02 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yishai Hadas X-Patchwork-Id: 10477453 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id BAC9C604D3 for ; Wed, 20 Jun 2018 13:21:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A675727DA4 for ; Wed, 20 Jun 2018 13:21:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 9AC0E2818E; Wed, 20 Jun 2018 13:21:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D38B127DA4 for ; Wed, 20 Jun 2018 13:21:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752870AbeFTNVH (ORCPT ); Wed, 20 Jun 2018 09:21:07 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:45926 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753644AbeFTNVG (ORCPT ); Wed, 20 Jun 2018 09:21:06 -0400 Received: by mail-wr0-f194.google.com with SMTP id o12-v6so3266379wrm.12 for ; Wed, 20 Jun 2018 06:21:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dev-mellanox-co-il.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=bSrb6YzkE8oQfYdVAngCfM3xLsofWOM/pgT4PtUKiYc=; b=jgsLbKkPytP2ZajzMeh/Wx2CkirqOmJrlXlRJRl5tBYCn1fjUk2zDLdYbgFyO0uVWI Zmsi/LK6TKKTMnYrfOkQm+97bc7jK+jZCDqt1FEF130JzJMhKBK5WQgXTDzok1Ilua15 zRzdC1PLjBVHtZS6CGfAEy4fBQOkmkDg3mZjNk2xAbwp8HtsokOyTmGH0Um7TGXIw89N yENYQJM/MGCklt9+nqbduSDmTZSkX2UScqSF+4b/F8SbaNgnSiRvQJmfXUUks1DxvA2T v2cHtPolkukOKHPaI1OAGZiwp6XWVZ2rP0+DccSPhsDE3VBzYj5cJ4vpQFRnjToP68uz +K+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bSrb6YzkE8oQfYdVAngCfM3xLsofWOM/pgT4PtUKiYc=; b=DW6dC4+vBbcSzcPi8hU/VkSpKMO+W6+0W8esLZcYOwKgc7DKPChk4VA9CuP/olYsNy H3a04RZJeTo8Ab29aC2gQVv3k04+AVL9h9XiN5ho4z6VxbrXRQzyMwz8w/G4tDyiD7Lj BMU4XjiPCBTwKeb34p4ZV6crPO5hg3yu9fEDWbAIE9aftnkOEouTlNC8tfSsstdH/pIK AZzc84iXtOpVhymWkYVpTUGMrQ6EwaZS50pFixAsMxN3Ob2N/lTJ21P/jBq+gjxE4LHl 0TxVEoY/rANuYih+0r511F3RcjC0lPNGl47vNbYaSoE3QEgwfBhr7OqhkoFoFq2cDWrf 4eWg== X-Gm-Message-State: APt69E2hkG+s2NqVHDtAiyZih7yxr9lUvGVFk1i2apTm7JQ1wKy2JA83 jbVa3hOnVaBVLVo/XZvSwy5qTA== X-Google-Smtp-Source: ADUXVKJK2Gvpw8bQXysThUQv2zGNOJ4yFuAVbp0SMUdnHcMXMh4uSSQgh+uRblmqI7QpsPcdgSoJVw== X-Received: by 2002:a5d:4b4b:: with SMTP id w11-v6mr17888539wrs.87.1529500865050; Wed, 20 Jun 2018 06:21:05 -0700 (PDT) Received: from [10.8.1.82] ([193.47.165.251]) by smtp.googlemail.com with ESMTPSA id l10-v6sm3179381wrm.29.2018.06.20.06.21.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 20 Jun 2018 06:21:04 -0700 (PDT) Subject: Re: [PATCH rdma-next v3] IB/core: Improve uverbs_cleanup_ucontext algorithm To: Jason Gunthorpe Cc: Leon Romanovsky , Doug Ledford , Yishai Hadas , RDMA mailing list , Leon Romanovsky References: <20180619160535.19904-1-leon@kernel.org> <20180619185315.GS27457@mellanox.com> From: Yishai Hadas Message-ID: <98f45368-0dcf-d68b-759c-7a6fae08e74a@dev.mellanox.co.il> Date: Wed, 20 Jun 2018 16:21:02 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180619185315.GS27457@mellanox.com> Content-Language: en-US Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 6/19/2018 9:53 PM, Jason Gunthorpe wrote: > On Tue, Jun 19, 2018 at 07:05:35PM +0300, Leon Romanovsky wrote: >> From: Yishai Hadas >> >> Improve uverbs_cleanup_ucontext algorithm to work properly even when >> there are two objects from the same type and one points to the other. >> For that case to work the 'destroy_order' is not used any more as it's >> static per type and can't support this use case. >> >> Instead, the new algorithm iterates over the objects in a LIFO mode as >> was before, at the end of each loop if were left objects that couldn't >> be destroyed it re-iterates over them give another chance to destroy them >> successfully. >> >> If was one iteration that didn't cleanup any object the last iteration >> will force the cleanup as was done before this change, this is coming to >> prevent memory leaks even in that fatal case. >> >> Signed-off-by: Yishai Hadas >> Signed-off-by: Leon Romanovsky >> --- >> drivers/infiniband/core/rdma_core.c | 111 ++++++++++++--------- >> drivers/infiniband/core/uverbs.h | 2 +- >> drivers/infiniband/core/uverbs_cmd.c | 6 +- >> drivers/infiniband/core/uverbs_std_types.c | 38 ++++--- >> .../infiniband/core/uverbs_std_types_counters.c | 4 +- >> drivers/infiniband/core/uverbs_std_types_cq.c | 4 +- >> drivers/infiniband/core/uverbs_std_types_dm.c | 5 +- >> .../infiniband/core/uverbs_std_types_flow_action.c | 4 +- >> drivers/infiniband/core/uverbs_std_types_mr.c | 3 +- >> include/rdma/ib_verbs.h | 15 ++- >> include/rdma/uverbs_types.h | 11 +- >> 11 files changed, 112 insertions(+), 91 deletions(-) > > Since devx is accepted now this will need to be respun to follow the > pattern in devx.c too. > Correct, V4 will set this pattern for devx as well. >> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c >> index df3c40533252..52dca36113dc 100644 >> --- a/drivers/infiniband/core/rdma_core.c >> +++ b/drivers/infiniband/core/rdma_core.c >> @@ -360,9 +360,10 @@ static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj, >> >> /* >> * We can only fail gracefully if the user requested to destroy the >> - * object. In the rest of the cases, just remove whatever you can. >> + * object or when a retry may be called upon an error. >> + * In the rest of the cases, just remove whatever you can. >> */ >> - if (why == RDMA_REMOVE_DESTROY && ret) >> + if (ret && ib_is_remove_retry(why, uobj)) >> return ret; > > I am wondering if this pattern should always read: > > if (ib_is_destroy_retryable(ret, why, uobj)) > return ret; > I'm fine with that pattern as well. > /* Otherwise proceed to force-destroy as much as possible, core code > will print a warning and leak the resources */ > > Which is a more regular.. > >> @@ -645,61 +646,73 @@ void uverbs_close_fd(struct file *f) >> kref_put(uverbs_file_ref, ib_uverbs_release_file); >> } >> >> -void uverbs_cleanup_ucontext(struct ib_ucontext *ucontext, bool device_removed) >> +static int __uverbs_cleanup_ucontext(struct ib_ucontext *ucontext, >> + enum rdma_remove_reason reason) >> { >> - enum rdma_remove_reason reason = device_removed ? >> - RDMA_REMOVE_DRIVER_REMOVE : RDMA_REMOVE_CLOSE; > > lets keep this hunk instead of the repeated expression.. > OK >> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c >> index 908ee8ab3297..d0226f41f0c7 100644 >> --- a/drivers/infiniband/core/uverbs_cmd.c >> +++ b/drivers/infiniband/core/uverbs_cmd.c >> @@ -116,6 +116,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, >> ucontext->tgid = get_task_pid(current->group_leader, PIDTYPE_PID); >> rcu_read_unlock(); >> ucontext->closing = 0; >> + ucontext->cleanup_retryable = false; > > Isn't ucontext kzalloc'd? It should be. > The allocation is done by each vendor, IB layer shouldn't rely on each of to do kzalloc. This is done for other fields here (e.g. ucontext->closing = 0), better stay with this. >> #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING >> ucontext->umem_tree = RB_ROOT_CACHED; >> @@ -611,12 +612,13 @@ ssize_t ib_uverbs_close_xrcd(struct ib_uverbs_file *file, >> return ret ?: in_len; >> } >> >> -int ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, >> +int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, >> struct ib_xrcd *xrcd, >> enum rdma_remove_reason why) >> { >> struct inode *inode; >> int ret; >> + struct ib_uverbs_device *dev = uobject->context->ufile->device; > >> inode = xrcd->inode; >> if (inode && !atomic_dec_and_test(&xrcd->usecnt)) >> @@ -624,7 +626,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, >> >> ret = ib_dealloc_xrcd(xrcd); >> >> - if (why == RDMA_REMOVE_DESTROY && ret) >> + if (ret && ib_is_remove_retry(why, uobject)) >> atomic_inc(&xrcd->usecnt); >> else if (inode) > > This would read alot better as > > if (ib_is_destroy_retryable(ret, why, uobj)) { > atomic_inc(&xrcd->usecnt); > return ret; > } > > if (inode) > xrcd_table_delete(dev, inode); > return 0; > >> xrcd_table_delete(dev, inode); OK >> diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c >> index 0df0ac9c1de3..4a468ef0ae72 100644 >> --- a/drivers/infiniband/core/uverbs_std_types.c >> +++ b/drivers/infiniband/core/uverbs_std_types.c >> @@ -74,7 +74,7 @@ static int uverbs_free_qp(struct ib_uobject *uobject, >> container_of(uobject, struct ib_uqp_object, uevent.uobject); >> int ret; >> >> - if (why == RDMA_REMOVE_DESTROY) { >> + if (ib_is_remove_retry(why, uobject)) { >> if (!list_empty(&uqp->mcast_list)) >> return -EBUSY; > > ugh, similar comment about the else. > OK >> diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c >> index 03b182a684a6..9aff3798e6fc 100644 >> --- a/drivers/infiniband/core/uverbs_std_types_counters.c >> +++ b/drivers/infiniband/core/uverbs_std_types_counters.c >> @@ -39,7 +39,7 @@ static int uverbs_free_counters(struct ib_uobject *uobject, >> { >> struct ib_counters *counters = uobject->object; >> >> - if (why == RDMA_REMOVE_DESTROY && >> + if (ib_is_remove_retry(why, uobject) && >> atomic_read(&counters->usecnt)) >> return -EBUSY; > > This is also quite a common pattern.. maybe even add the usecnt: > > ib_is_destroy_retryable(ret, why, uobj, &counters->usecnt) > > ? This is not a fully common pattern, I prefer leaving this out of ib_is_destroy_retryable() and consider the 'usecnt' as part of the 'ret' value when it's applicable. For the above: } >> --- a/drivers/infiniband/core/uverbs_std_types_cq.c >> +++ b/drivers/infiniband/core/uverbs_std_types_cq.c >> @@ -44,7 +44,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject, >> int ret; >> >> ret = ib_destroy_cq(cq); >> - if (!ret || why != RDMA_REMOVE_DESTROY) >> + if (!ret || !ib_is_remove_retry(why, uobject)) >> ib_uverbs_release_ucq(uobject->context->ufile, ev_queue ? >> container_of(ev_queue, >> struct ib_uverbs_completion_event_file, > > The (existing) use of ! in the if is ugly, should be changed. > Will clean it up. >> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h >> index dc5d262739e5..1b17fa8a2d86 100644 >> --- a/include/rdma/ib_verbs.h >> +++ b/include/rdma/ib_verbs.h >> @@ -1476,7 +1476,10 @@ struct ib_fmr_attr { >> struct ib_umem; >> > >> struct pid *tgid; >> #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING >> @@ -2684,6 +2688,15 @@ static inline bool ib_is_udata_cleared(struct ib_udata *udata, >> return ib_is_buffer_cleared(udata->inbuf + offset, len); >> } >> >> +static inline bool ib_is_remove_retry(enum rdma_remove_reason why, >> + struct ib_uobject *uobj) >> +{ >> + if (why == RDMA_REMOVE_DESTROY || uobj->context->cleanup_retryable) >> + return true; >> + >> + return false; >> +} > > I think a cocinelle script will complain this should just be > > return why == RDMA_REMOVE_DESTROY || uobj->context->cleanup_retryable; > > Also add a kdoc comment explaining what the expected use is. Sure, will handle. --- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/drivers/infiniband/core/uverbs_std_types_counters.c +++ b/drivers/infiniband/core/uverbs_std_types_counters.c @@ -38,10 +38,10 @@ static int uverbs_free_counters(struct ib_uobject *uobject, enum rdma_remove_reason why) { struct ib_counters *counters = uobject->object; + int ret = atomic_read(&counters->usecnt) ? -EBUSY : 0; - if (why == RDMA_REMOVE_DESTROY && - atomic_read(&counters->usecnt)) - return -EBUSY; + if (ib_is_destroy_retryable(ret, why, uobject)) + return ret; return counters->device->destroy_counters(counters);