All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
To: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	"Michael J. Ruhl"
	<michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	Mike Marciniszyn
	<mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
	stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: [PATCH 07/15] IB/hfi1: Fix a subcontext memory leak
Date: Tue, 02 May 2017 17:41:41 -0700	[thread overview]
Message-ID: <20170503004140.6965.90763.stgit@scvm10.sc.intel.com> (raw)
In-Reply-To: <20170503003734.6965.67405.stgit-9QXIwq+3FY+1XWohqUldA0EOCMrvLtNR@public.gmane.org>

From: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

The only context that frees user_exp_rcv data structures is the last
context closed (from a sub-context set).  This leaks the allocations
from the other sub-contexts.  Separate the common frees from the
specific frees and call them at the appropriate time.

Using KEDR to check for memory leaks we get:

Before test:

[leak_check] Possible leaks: 25

After test:

[leak_check] Possible leaks: 31  (6 leaked data structures)

After patch applied (before and after test have the same value)

[leak_check] Possible leaks: 25

Each leak is 192 + 13440 + 6720 = 20352 bytes per sub-context.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Reviewed-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/hfi1/file_ops.c     |    5 ++++-
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   32 ++++++++++++++++-------------
 drivers/infiniband/hw/hfi1/user_exp_rcv.h |    1 +
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 3d9bce4..afdf3ef 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -756,6 +756,9 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 	/* release the cpu */
 	hfi1_put_proc_affinity(fdata->rec_cpu_num);
 
+	/* clean up rcv side */
+	hfi1_user_exp_rcv_free(fdata);
+
 	/*
 	 * Clear any left over, unhandled events so the next process that
 	 * gets this context doesn't get confused.
@@ -795,7 +798,7 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 
 	dd->rcd[uctxt->ctxt] = NULL;
 
-	hfi1_user_exp_rcv_free(fdata);
+	hfi1_user_exp_rcv_grp_free(uctxt);
 	hfi1_clear_ctxt_pkey(dd, uctxt->ctxt);
 
 	uctxt->rcvwait_to = 0;
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 35c6e7e..c7f13df 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -252,36 +252,40 @@ int hfi1_user_exp_rcv_init(struct file *fp)
 	return ret;
 }
 
+void hfi1_user_exp_rcv_grp_free(struct hfi1_ctxtdata *uctxt)
+{
+	struct tid_group *grp, *gptr;
+
+	list_for_each_entry_safe(grp, gptr, &uctxt->tid_group_list.list,
+				 list) {
+		list_del_init(&grp->list);
+		kfree(grp);
+	}
+	hfi1_clear_tids(uctxt);
+}
+
 int hfi1_user_exp_rcv_free(struct hfi1_filedata *fd)
 {
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
-	struct tid_group *grp, *gptr;
 
-	if (!test_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags))
-		return 0;
 	/*
 	 * The notifier would have been removed when the process'es mm
 	 * was freed.
 	 */
-	if (fd->handler)
+	if (fd->handler) {
 		hfi1_mmu_rb_unregister(fd->handler);
-
-	kfree(fd->invalid_tids);
-
-	if (!uctxt->cnt) {
+	} else {
 		if (!EXP_TID_SET_EMPTY(uctxt->tid_full_list))
 			unlock_exp_tids(uctxt, &uctxt->tid_full_list, fd);
 		if (!EXP_TID_SET_EMPTY(uctxt->tid_used_list))
 			unlock_exp_tids(uctxt, &uctxt->tid_used_list, fd);
-		list_for_each_entry_safe(grp, gptr, &uctxt->tid_group_list.list,
-					 list) {
-			list_del_init(&grp->list);
-			kfree(grp);
-		}
-		hfi1_clear_tids(uctxt);
 	}
 
+	kfree(fd->invalid_tids);
+	fd->invalid_tids = NULL;
+
 	kfree(fd->entry_to_rb);
+	fd->entry_to_rb = NULL;
 	return 0;
 }
 
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.h b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
index 9bc8d9f..d1d7d3d 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
@@ -70,6 +70,7 @@
 		(tid) |= EXP_TID_SET(field, (value));			\
 	} while (0)
 
+void hfi1_user_exp_rcv_grp_free(struct hfi1_ctxtdata *uctxt);
 int hfi1_user_exp_rcv_init(struct file *);
 int hfi1_user_exp_rcv_free(struct hfi1_filedata *);
 int hfi1_user_exp_rcv_setup(struct file *, struct hfi1_tid_info *);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID (diff)
From: Dennis Dalessandro <dennis.dalessandro@intel.com>
To: dledford@redhat.com
Cc: linux-rdma@vger.kernel.org,
	"Michael J. Ruhl" <michael.j.ruhl@intel.com>,
	Mike Marciniszyn <mike.marciniszyn@intel.com>,
	stable@vger.kernel.org
Subject: [PATCH 07/15] IB/hfi1: Fix a subcontext memory leak
Date: Tue, 02 May 2017 17:41:41 -0700	[thread overview]
Message-ID: <20170503004140.6965.90763.stgit@scvm10.sc.intel.com> (raw)
In-Reply-To: <20170503003734.6965.67405.stgit@scvm10.sc.intel.com>

From: Michael J. Ruhl <michael.j.ruhl@intel.com>

The only context that frees user_exp_rcv data structures is the last
context closed (from a sub-context set).  This leaks the allocations
from the other sub-contexts.  Separate the common frees from the
specific frees and call them at the appropriate time.

Using KEDR to check for memory leaks we get:

Before test:

[leak_check] Possible leaks: 25

After test:

[leak_check] Possible leaks: 31  (6 leaked data structures)

After patch applied (before and after test have the same value)

[leak_check] Possible leaks: 25

Each leak is 192 + 13440 + 6720 = 20352 bytes per sub-context.

Cc: stable@vger.kernel.org
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/file_ops.c     |    5 ++++-
 drivers/infiniband/hw/hfi1/user_exp_rcv.c |   32 ++++++++++++++++-------------
 drivers/infiniband/hw/hfi1/user_exp_rcv.h |    1 +
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index 3d9bce4..afdf3ef 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -756,6 +756,9 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 	/* release the cpu */
 	hfi1_put_proc_affinity(fdata->rec_cpu_num);
 
+	/* clean up rcv side */
+	hfi1_user_exp_rcv_free(fdata);
+
 	/*
 	 * Clear any left over, unhandled events so the next process that
 	 * gets this context doesn't get confused.
@@ -795,7 +798,7 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 
 	dd->rcd[uctxt->ctxt] = NULL;
 
-	hfi1_user_exp_rcv_free(fdata);
+	hfi1_user_exp_rcv_grp_free(uctxt);
 	hfi1_clear_ctxt_pkey(dd, uctxt->ctxt);
 
 	uctxt->rcvwait_to = 0;
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 35c6e7e..c7f13df 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -252,36 +252,40 @@ int hfi1_user_exp_rcv_init(struct file *fp)
 	return ret;
 }
 
+void hfi1_user_exp_rcv_grp_free(struct hfi1_ctxtdata *uctxt)
+{
+	struct tid_group *grp, *gptr;
+
+	list_for_each_entry_safe(grp, gptr, &uctxt->tid_group_list.list,
+				 list) {
+		list_del_init(&grp->list);
+		kfree(grp);
+	}
+	hfi1_clear_tids(uctxt);
+}
+
 int hfi1_user_exp_rcv_free(struct hfi1_filedata *fd)
 {
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
-	struct tid_group *grp, *gptr;
 
-	if (!test_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags))
-		return 0;
 	/*
 	 * The notifier would have been removed when the process'es mm
 	 * was freed.
 	 */
-	if (fd->handler)
+	if (fd->handler) {
 		hfi1_mmu_rb_unregister(fd->handler);
-
-	kfree(fd->invalid_tids);
-
-	if (!uctxt->cnt) {
+	} else {
 		if (!EXP_TID_SET_EMPTY(uctxt->tid_full_list))
 			unlock_exp_tids(uctxt, &uctxt->tid_full_list, fd);
 		if (!EXP_TID_SET_EMPTY(uctxt->tid_used_list))
 			unlock_exp_tids(uctxt, &uctxt->tid_used_list, fd);
-		list_for_each_entry_safe(grp, gptr, &uctxt->tid_group_list.list,
-					 list) {
-			list_del_init(&grp->list);
-			kfree(grp);
-		}
-		hfi1_clear_tids(uctxt);
 	}
 
+	kfree(fd->invalid_tids);
+	fd->invalid_tids = NULL;
+
 	kfree(fd->entry_to_rb);
+	fd->entry_to_rb = NULL;
 	return 0;
 }
 
diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.h b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
index 9bc8d9f..d1d7d3d 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.h
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.h
@@ -70,6 +70,7 @@
 		(tid) |= EXP_TID_SET(field, (value));			\
 	} while (0)
 
+void hfi1_user_exp_rcv_grp_free(struct hfi1_ctxtdata *uctxt);
 int hfi1_user_exp_rcv_init(struct file *);
 int hfi1_user_exp_rcv_free(struct hfi1_filedata *);
 int hfi1_user_exp_rcv_setup(struct file *, struct hfi1_tid_info *);

  parent reply	other threads:[~2017-05-03  0:41 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-03  0:40 [PATCH 00/15] IB/hfi1: hfi1 driver patches for 4.12 Dennis Dalessandro
2017-05-03  0:40 ` Dennis Dalessandro
2017-05-03  0:41 ` [PATCH 06/15] IB/hfi1: Return an error on memory allocation failure Dennis Dalessandro
     [not found] ` <20170503003734.6965.67405.stgit-9QXIwq+3FY+1XWohqUldA0EOCMrvLtNR@public.gmane.org>
2017-05-03  0:41   ` [PATCH 01/15] IB/hfi1: Fix checks for Offline transient state Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 02/15] IB/hfi1, IB/rdmavt: Move r_adefered to r_lock cache line Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 03/15] IB/hfi1: Fix yield logic in send engine Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 04/15] IB/hfi1: Get rid of divide when setting the tx request header Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 05/15] IB/hfi1: Adjust default eager_buffer_size to 8MB Dennis Dalessandro
2017-05-03  0:41   ` Dennis Dalessandro [this message]
2017-05-03  0:41     ` [PATCH 07/15] IB/hfi1: Fix a subcontext memory leak Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 08/15] IB/hfi1: Name function prototype parameters Dennis Dalessandro
2017-05-03  0:41   ` [PATCH 09/15] IB/hfi1: Use filedata rather than filepointer Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 10/15] IB/hfi1: Remove atomic operations for SDMA_REQ_HAVE_AHG bit Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 11/15] IB/hfi1: Search shared contexts on the opened device, not all devices Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 12/15] IB/hfi1: Correctly clear the pkey Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 13/15] IB/hfi1: Clean up context initialization Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 14/15] IB/hfi1: Fix an assign/ordering issue with shared context IDs Dennis Dalessandro
     [not found]     ` <20170503004223.6965.13575.stgit-9QXIwq+3FY+1XWohqUldA0EOCMrvLtNR@public.gmane.org>
2017-05-03  7:36       ` Leon Romanovsky
     [not found]         ` <20170503073632.GD22833-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
2017-05-03 11:19           ` Dennis Dalessandro
2017-05-03  0:42   ` [PATCH 15/15] IB/hfi1: Clean up on context initialization failure Dennis Dalessandro
     [not found]     ` <20170503004229.6965.97776.stgit-9QXIwq+3FY+1XWohqUldA0EOCMrvLtNR@public.gmane.org>
2017-05-03  7:35       ` Leon Romanovsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170503004140.6965.90763.stgit@scvm10.sc.intel.com \
    --to=dennis.dalessandro-ral2jqcrhueavxtiumwx3w@public.gmane.org \
    --cc=dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
    --cc=linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=michael.j.ruhl-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
    --cc=stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.