Re: [PATCH v3] IB/umem: Release pid in error and ODP flow

[Leon Romanovsky <leon@kernel.org>]

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

On Wed, Jan 04, 2017 at 03:07:04PM +0800, Kenneth Lee wrote:
> On Tue, Jan 03, 2017 at 12:12:24PM +0200, Leon Romanovsky wrote:
> > Date: Tue, 3 Jan 2017 12:12:24 +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 v3] IB/umem: Release pid in error and ODP flow
> > User-Agent: Mutt/1.7.2 (2016-11-26)
> > Message-ID: <20170103101224.GH12077@mtr-leonro.local>
> >
> > On Tue, Jan 03, 2017 at 10:32:50AM +0800, Kenneth Lee wrote:
> > > On Sun, Jan 01, 2017 at 08:47:12AM +0200, Leon Romanovsky wrote:
> > > > Date: Sun, 1 Jan 2017 08:47:12 +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 v3] IB/umem: Release pid in error and ODP flow
> > > > User-Agent: Mutt/1.7.2 (2016-11-26)
> > > > Message-ID: <20170101064712.GQ26885@mtr-leonro.local>
> > > >
> > > > On Fri, Dec 30, 2016 at 06:18:29PM +0800, Kenneth Lee wrote:
> > > > > There are two bugfixes in this patch:
> > > > >
> > > > > Fixes: 87773dd56d54 ("IB: ib_umem_release() should decrement mm->pinned_vm from ib_umem_get")
> > > > > 	This patch introduce the get_task_pid but not put it back on all error
> > > > > 	path
> > > > >
> > > > > Fixes: 8ada2c1c0c1d ("IB/core: Add support for on demand paging regions")
> > > > > 	This patch introduce a ODP flow without release pid before enter it
> > > > >
> > > > >
> > > > > 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
> > > > > Change from v2 to v3:
> > > > >   Update the title and add "Fixes" fields in the description
> > > >
> > > > OK,
> > > >
> > > > I see that you still didn't read Documentation/SubmittingPatches. You
> > > > must read that document before you are sending patches.
> > > >
> > > > But I'll stop here, the code is correct (it fixes bugs) and commit message
> > > > more usefull than before.
> > > >
> > > >
> > > > >
> > > > >  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
> > > > >
> > > > > --
> > > > > 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
> > >
> > > Thanks,
> > >
> > > I did read the doc, but maybe I mis-understant some points. Could you please
> > > point it out?
> >
> > Fixes line should be placed above bottom signatures.
> >
> > As an example of properly written patch, you can take a look on the
> > following patch [1] from Steve.
> >
> > [1] http://marc.info/?l=linux-rdma&m=148244272205411&w=2
>
> Thank you. A sample help a lot.
>
> But please allow me to argue a little:
> Documentation/process/submitting-patches.rst does really not mention where Fixes
> tags should be put:)

Sure, you can't and don't want to document everything. There is section named
"2) Describe your changes" which has very extensive description about
commit message. If you had followed it, you would find that it is very
natural to place Fixes at the bottom.

Thanks

>
> >
> > >
> > > And sorry. please ignore the last message. I forget to use a bottom-post style.
> > >
> > >
> > >
> > > --
> > > 			-Kenneth(Hisilicon)
>
>
>
> --
> 			-Kenneth(Hisilicon)

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