Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

[Leon Romanovsky]

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

On Thu, Dec 29, 2016 at 04:27:28PM +0800, Kenneth Lee wrote:
> There are two bugfixes in this patch:
>
> 1. When the execution go to the ib_umem_odp_get() path, pid should be put
>    back.
> 2. When the memory allocation fail, the pid also should be put back before
>    exit.
>
> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> Reviewed-by: Haggai Eran <haggaie@mellanox.com>
> ---
> Change from v1 to v2:
>   Correcting the patch title and description

I don't see any changes except version in the title.
What about anything like this?
[PATCH v3] IB/umem: Release pid in error and ODP flows

And Fixes line please, it will help to forward it to stable trees.

Thanks

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

[PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

[Kenneth Lee]

There are two bugfixes in this patch:

1. When the execution go to the ib_umem_odp_get() path, pid should be put
   back.
2. When the memory allocation fail, the pid also should be put back before
   exit.

Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
Reviewed-by: Haggai Eran <haggaie@mellanox.com>
---
Change from v1 to v2:
  Correcting the patch title and description

 drivers/infiniband/core/umem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 1e62a5f..4609b92 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -134,6 +134,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 		 IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_MW_BIND));
 
 	if (access & IB_ACCESS_ON_DEMAND) {
+		put_pid(umem->pid);
 		ret = ib_umem_odp_get(context, umem);
 		if (ret) {
 			kfree(umem);
@@ -149,6 +150,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
 
 	page_list = (struct page **) __get_free_page(GFP_KERNEL);
 	if (!page_list) {
+		put_pid(umem->pid);
 		kfree(umem);
 		return ERR_PTR(-ENOMEM);
 	}
-- 
1.9.1

Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

[Kenneth Lee]

Hi, Leon,

1. I do change the title except for the version number itself:) But my English
is quite bad, maybe the title is still quite stupid. I can update it according
to your advice.

2. I catched the bug by reading the final code, not by bisect-ing the old
commit. Do you means I should find out which commit introducing the bug? It will
not be easily to say which it is because it is a "missing bug", rather than a
"introduced bug". Indicate the commit may not help to remove a patch/commit from
the stable tree.

Could you please give more suggestion? Thanks.

On Thu, Dec 29, 2016 at 10:17:56AM +0200, Leon Romanovsky wrote:
> Date: Thu, 29 Dec 2016 10:17:56 +0200
> From: Leon Romanovsky <leon@kernel.org>
> To: Kenneth Lee <liguozhu@hisilicon.com>
> CC: dledford@redhat.com, sean.hefty@intel.com, hal.rosenstock@gmail.com,
>  robin.murphy@arm.com, jroedel@suse.de, egtvedt@samfundet.no,
>  vgupta@synopsys.com, dave.hansen@linux.intel.com, lstoakes@gmail.com,
>  krzk@kernel.org, sebott@linux.vnet.ibm.com, markb@mellanox.com,
>  linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()
> User-Agent: Mutt/1.7.2 (2016-11-26)
> Message-ID: <20161229081756.GI26885@mtr-leonro.local>
> 
> On Thu, Dec 29, 2016 at 04:27:28PM +0800, Kenneth Lee wrote:
> > There are two bugfixes in this patch:
> >
> > 1. When the execution go to the ib_umem_odp_get() path, pid should be put
> >    back.
> > 2. When the memory allocation fail, the pid also should be put back before
> >    exit.
> >
> > Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> > Reviewed-by: Haggai Eran <haggaie@mellanox.com>
> > ---
> > Change from v1 to v2:
> >   Correcting the patch title and description
> 
> I don't see any changes except version in the title.
> What about anything like this?
> [PATCH v3] IB/umem: Release pid in error and ODP flows
> 
> And Fixes line please, it will help to forward it to stable trees.
> 
> Thanks



-- 
			-Kenneth(Hisilicon)

Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

[Leon Romanovsky]

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

On Fri, Dec 30, 2016 at 12:50:11PM +0800, Kenneth Lee wrote:
> Hi, Leon,
>
> 1. I do change the title except for the version number itself:) But my English
> is quite bad, maybe the title is still quite stupid. I can update it according
> to your advice.

Yes, please
The main points are:
1. Remove "bugifix", it is not needed.
2. Use description in the title and not function names.

>
> 2. I catched the bug by reading the final code, not by bisect-ing the old
> commit. Do you means I should find out which commit introducing the bug? It will
> not be easily to say which it is because it is a "missing bug", rather than a
> "introduced bug". Indicate the commit may not help to remove a patch/commit from
> the stable tree.

The fixes line won't cause for removal of commit, but to addition of
yours on top of their code base.

git blame is your friend.

one fixes line is:
Fixes: 8ada2c1c0c1d ("IB/core: Add support for on demand paging regions")

and the second line is !!!!! NOT !!!!!, you need to go deeper in the logs !!!!!!
Fixes: f7c6a7b5d599 ("IB/uverbs: Export ib_umem_get()/ib_umem_release() to modules")

>
> Could you please give more suggestion? Thanks.

Please, don't use top-posting for this mailing list.
It is really-really annoying.

>
> On Thu, Dec 29, 2016 at 10:17:56AM +0200, Leon Romanovsky wrote:
> > Date: Thu, 29 Dec 2016 10:17:56 +0200
> > From: Leon Romanovsky <leon@kernel.org>
> > To: Kenneth Lee <liguozhu@hisilicon.com>
> > CC: dledford@redhat.com, sean.hefty@intel.com, hal.rosenstock@gmail.com,
> >  robin.murphy@arm.com, jroedel@suse.de, egtvedt@samfundet.no,
> >  vgupta@synopsys.com, dave.hansen@linux.intel.com, lstoakes@gmail.com,
> >  krzk@kernel.org, sebott@linux.vnet.ibm.com, markb@mellanox.com,
> >  linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()
> > User-Agent: Mutt/1.7.2 (2016-11-26)
> > Message-ID: <20161229081756.GI26885@mtr-leonro.local>
> >
> > On Thu, Dec 29, 2016 at 04:27:28PM +0800, Kenneth Lee wrote:
> > > There are two bugfixes in this patch:
> > >
> > > 1. When the execution go to the ib_umem_odp_get() path, pid should be put
> > >    back.
> > > 2. When the memory allocation fail, the pid also should be put back before
> > >    exit.
> > >
> > > Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> > > Reviewed-by: Haggai Eran <haggaie@mellanox.com>
> > > ---
> > > Change from v1 to v2:
> > >   Correcting the patch title and description
> >
> > I don't see any changes except version in the title.
> > What about anything like this?
> > [PATCH v3] IB/umem: Release pid in error and ODP flows
> >
> > And Fixes line please, it will help to forward it to stable trees.
> >
> > Thanks
>
>
>
> --
> 			-Kenneth(Hisilicon)

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

Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()

[Kenneth Lee]

On Fri, Dec 30, 2016 at 08:55:10AM +0200, Leon Romanovsky wrote:
> Date: Fri, 30 Dec 2016 08:55:10 +0200
> From: Leon Romanovsky <leon@kernel.org>
> To: Kenneth Lee <liguozhu@hisilicon.com>
> CC: dledford@redhat.com, sean.hefty@intel.com, hal.rosenstock@gmail.com,
>  robin.murphy@arm.com, jroedel@suse.de, egtvedt@samfundet.no,
>  vgupta@synopsys.com, dave.hansen@linux.intel.com, lstoakes@gmail.com,
>  krzk@kernel.org, sebott@linux.vnet.ibm.com, markb@mellanox.com,
>  linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()
> User-Agent: Mutt/1.7.2 (2016-11-26)
> Message-ID: <20161230065510.GL26885@mtr-leonro.local>
> 
> On Fri, Dec 30, 2016 at 12:50:11PM +0800, Kenneth Lee wrote:
> > Hi, Leon,
> >
> > 1. I do change the title except for the version number itself:) But my English
> > is quite bad, maybe the title is still quite stupid. I can update it according
> > to your advice.
> 
> Yes, please
> The main points are:
> 1. Remove "bugifix", it is not needed.
> 2. Use description in the title and not function names.
> 
> >
> > 2. I catched the bug by reading the final code, not by bisect-ing the old
> > commit. Do you means I should find out which commit introducing the bug? It will
> > not be easily to say which it is because it is a "missing bug", rather than a
> > "introduced bug". Indicate the commit may not help to remove a patch/commit from
> > the stable tree.
> 
> The fixes line won't cause for removal of commit, but to addition of
> yours on top of their code base.
> 
> git blame is your friend.
> 
> one fixes line is:
> Fixes: 8ada2c1c0c1d ("IB/core: Add support for on demand paging regions")
> 
> and the second line is !!!!! NOT !!!!!, you need to go deeper in the logs !!!!!!
> Fixes: f7c6a7b5d599 ("IB/uverbs: Export ib_umem_get()/ib_umem_release() to modules")
> 
> >
> > Could you please give more suggestion? Thanks.
> 
> Please, don't use top-posting for this mailing list.
> It is really-really annoying.
> 
> >
> > On Thu, Dec 29, 2016 at 10:17:56AM +0200, Leon Romanovsky wrote:
> > > Date: Thu, 29 Dec 2016 10:17:56 +0200
> > > From: Leon Romanovsky <leon@kernel.org>
> > > To: Kenneth Lee <liguozhu@hisilicon.com>
> > > CC: dledford@redhat.com, sean.hefty@intel.com, hal.rosenstock@gmail.com,
> > >  robin.murphy@arm.com, jroedel@suse.de, egtvedt@samfundet.no,
> > >  vgupta@synopsys.com, dave.hansen@linux.intel.com, lstoakes@gmail.com,
> > >  krzk@kernel.org, sebott@linux.vnet.ibm.com, markb@mellanox.com,
> > >  linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v2] ib umem: bugfix: mixed put_pid()s in ib_umem_get()
> > > User-Agent: Mutt/1.7.2 (2016-11-26)
> > > Message-ID: <20161229081756.GI26885@mtr-leonro.local>
> > >
> > > On Thu, Dec 29, 2016 at 04:27:28PM +0800, Kenneth Lee wrote:
> > > > There are two bugfixes in this patch:
> > > >
> > > > 1. When the execution go to the ib_umem_odp_get() path, pid should be put
> > > >    back.
> > > > 2. When the memory allocation fail, the pid also should be put back before
> > > >    exit.
> > > >
> > > > Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com>
> > > > Reviewed-by: Haggai Eran <haggaie@mellanox.com>
> > > > ---
> > > > Change from v1 to v2:
> > > >   Correcting the patch title and description
> > >
> > > I don't see any changes except version in the title.
> > > What about anything like this?
> > > [PATCH v3] IB/umem: Release pid in error and ODP flows
> > >
> > > And Fixes line please, it will help to forward it to stable trees.
> > >
> > > Thanks
> >
> >
> >
> > --
> > 			-Kenneth(Hisilicon)

Very helpful. Thank you. I will send the Patch v3 soon.

-- 
			-Kenneth(Hisilicon)