[PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
Add datastructure for and allocation/deallocation of protection domains for
RDMAVT.
Reviewed-by: Ira Weiny
Reviewed-by: Mike Marciniszyn
Signed-off-by: Dennis Dalessandro
---
drivers/infiniband/sw/rdmavt/Makefile |3 +
drivers/infiniband/sw/rdmavt/pd.c | 106 +
drivers/infiniband/sw/rdmavt/pd.h | 61 +++
drivers/infiniband/sw/rdmavt/vt.c | 12
drivers/infiniband/sw/rdmavt/vt.h |1
include/rdma/rdma_vt.h| 34 +--
6 files changed, 212 insertions(+), 5 deletions(-)
create mode 100644 drivers/infiniband/sw/rdmavt/pd.c
create mode 100644 drivers/infiniband/sw/rdmavt/pd.h
diff --git a/drivers/infiniband/sw/rdmavt/Makefile
b/drivers/infiniband/sw/rdmavt/Makefile
index 134d2d0..c6751bb 100644
--- a/drivers/infiniband/sw/rdmavt/Makefile
+++ b/drivers/infiniband/sw/rdmavt/Makefile
@@ -7,4 +7,5 @@
#
obj-$(CONFIG_INFINIBAND_RDMAVT) += rdmavt.o
-rdmavt-y := vt.o dma.o
+rdmavt-y := vt.o dma.o pd.o
+
diff --git a/drivers/infiniband/sw/rdmavt/pd.c
b/drivers/infiniband/sw/rdmavt/pd.c
new file mode 100644
index 000..2f7a97f
--- /dev/null
+++ b/drivers/infiniband/sw/rdmavt/pd.c
@@ -0,0 +1,106 @@
+/*
+ *
+ * This file is provided under a dual BSD/GPLv2 license. When using or
+ * redistributing this file, you may do so under either license.
+ *
+ * GPL LICENSE SUMMARY
+ *
+ * Copyright(c) 2015 Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * BSD LICENSE
+ *
+ * Copyright(c) 2015 Intel Corporation.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * - 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.
+ * - Neither the name of Intel Corporation nor the names of its
+ *contributors may be used to endorse or promote products derived
+ *from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS 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 COPYRIGHT
+ * OWNER 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.
+ *
+ */
+
+#include
+#include "pd.h"
+
+struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
+ struct ib_ucontext *context,
+ struct ib_udata *udata)
+{
+ struct rvt_dev_info *dev = ib_to_rvt(ibdev);
+ struct rvt_pd *pd;
+ struct ib_pd *ret;
+
+ pd = kmalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd) {
+ ret = ERR_PTR(-ENOMEM);
+ goto bail;
+ }
+ /*
+* This is actually totally arbitrary. Some correctness tests
+* assume there's a maximum number of PDs that can be allocated.
+* We don't actually have this limit, but we fail the test if
+* we allow allocations of more than we report for this value.
+*/
+
+ spin_lock(&dev->n_pds_lock);
+ if (dev->n_pds_allocated == dev->dparms.max_pds) {
+ spin_unlock(&dev->n_pds_lock);
+ kfree(pd);
+ ret = ERR_PTR(-ENOMEM);
+ goto bail;
+ }
+
+ dev->n_pds_allocated++;
+ spin_unlock(&dev->n_pds_lock);
+
+ /* ib_alloc_pd() will initialize pd->ibpd. */
+ pd->user = udata ? 1 : 0;
+
+ ret = &pd->ibpd;
+
+bail:
+ return ret;
+}
+
+int rvt_dealloc_pd(struct ib_pd *ibpd)
+{
+ struct rvt_pd *pd = ibpd_to_rvtpd(ibpd);
+ struct rvt_dev_info *dev = ib_to_rvt(ibpd->device);
+
+ spin_lock(&dev->n_pds_lock);
+ dev->n_pds_allocated--;
+ spin_unlock(&dev->n_pds_lock);
+
RE: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
> +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> +struct ib_ucontext *context,
> +struct ib_udata *udata)
> +{
> + struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> + struct rvt_pd *pd;
> + struct ib_pd *ret;
> +
> + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd) {
> + ret = ERR_PTR(-ENOMEM);
> + goto bail;
> + }
> + /*
> + * This is actually totally arbitrary. Some correctness tests
> + * assume there's a maximum number of PDs that can be allocated.
> + * We don't actually have this limit, but we fail the test if
> + * we allow allocations of more than we report for this value.
> + */
Why not trap this in user space, rather than forcing the kernel to support some
test program?
N�r��yb�X��ǧv�^�){.n�+{��ٚ�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Mon, Dec 07, 2015 at 03:43:10PM -0500, Dennis Dalessandro wrote:
> +
> +/*
> + * Things that are driver specific, module parameters in hfi1 and qib
> + */
> +struct rvt_driver_params {
> + int max_pds;
Can it be negative value?
> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Tue, Dec 08, 2015 at 08:28:17AM +0200, Leon Romanovsky wrote:
> On Mon, Dec 07, 2015 at 03:43:10PM -0500, Dennis Dalessandro wrote:
> > +
> > +/*
> > + * Things that are driver specific, module parameters in hfi1 and qib
> > + */
> > +struct rvt_driver_params {
> > + int max_pds;
> Can it be negative value?
> > +};
Forget my question, I see, you removed this variable in the following patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Mon, Dec 07, 2015 at 03:13:16PM -0600, Sean Hefty wrote:
> > +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> > + struct ib_ucontext *context,
> > + struct ib_udata *udata)
> > +{
> > + struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> > + struct rvt_pd *pd;
> > + struct ib_pd *ret;
> > +
> > + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> > + if (!pd) {
> > + ret = ERR_PTR(-ENOMEM);
> > + goto bail;
> > + }
> > + /*
> > +* This is actually totally arbitrary. Some correctness tests
> > +* assume there's a maximum number of PDs that can be allocated.
> > +* We don't actually have this limit, but we fail the test if
> > +* we allow allocations of more than we report for this value.
> > +*/
>
> Why not trap this in user space, rather than forcing the kernel to support
> some test program?
>
What do you mean "trap this in user space"? This code is not supporting any
particular test program.
If users try and allocate more protection domains then are advertised then an
error should be returned.
Perhaps the comment should be removed if it is confusing?
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
> > > +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> > > +struct ib_ucontext *context,
> > > +struct ib_udata *udata)
> > > +{
> > > + struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> > > + struct rvt_pd *pd;
> > > + struct ib_pd *ret;
> > > +
> > > + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> > > + if (!pd) {
> > > + ret = ERR_PTR(-ENOMEM);
> > > + goto bail;
> > > + }
> > > + /*
> > > + * This is actually totally arbitrary. Some correctness tests
> > > + * assume there's a maximum number of PDs that can be allocated.
> > > + * We don't actually have this limit, but we fail the test if
> > > + * we allow allocations of more than we report for this value.
> > > + */
> >
> > Why not trap this in user space, rather than forcing the kernel to
> support some test program?
> >
>
> What do you mean "trap this in user space"? This code is not supporting
> any
> particular test program.
>
> If users try and allocate more protection domains then are advertised then
> an
> error should be returned.
>
> Perhaps the comment should be removed if it is confusing?
There is no theoretical limit on the number of PDs, or likely most other
resources. So why define and enforce an arbitrary limit? The justification
given was that some test program would fail. If there's a concern about that
test program passing, then enforce the limit in user space.
I didn't find the comment confusing. Just the choice to let a test app drive
the design.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Tue, Dec 08, 2015 at 01:17:48PM -0600, Sean Hefty wrote:
> > > > +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> > > > + struct ib_ucontext *context,
> > > > + struct ib_udata *udata)
> > > > +{
> > > > + struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> > > > + struct rvt_pd *pd;
> > > > + struct ib_pd *ret;
> > > > +
> > > > + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> > > > + if (!pd) {
> > > > + ret = ERR_PTR(-ENOMEM);
> > > > + goto bail;
> > > > + }
> > > > + /*
> > > > +* This is actually totally arbitrary. Some correctness tests
> > > > +* assume there's a maximum number of PDs that can be allocated.
> > > > +* We don't actually have this limit, but we fail the test if
> > > > +* we allow allocations of more than we report for this value.
> > > > +*/
> > >
> > > Why not trap this in user space, rather than forcing the kernel to
> > support some test program?
> > >
> >
> > What do you mean "trap this in user space"? This code is not supporting
> > any
> > particular test program.
> >
> > If users try and allocate more protection domains then are advertised then
> > an
> > error should be returned.
> >
> > Perhaps the comment should be removed if it is confusing?
>
> There is no theoretical limit on the number of PDs, or likely most other
> resources. So why define and enforce an arbitrary limit? The justification
> given was that some test program would fail. If there's a concern about that
> test program passing, then enforce the limit in user space.
I did not interpret this as being driven by a test but rather by the IBTA spec.
This may be pedantic but, wouldn't it be confusing from a user POV to see an
advertised limit but then not be constrained by it? I'm not opposed to setting
this limit arbitrarily high, but I still think the implementation should
enforce that limit.
>
> I didn't find the comment confusing. Just the choice to let a test app drive
> the design.
Perhaps "confusing" is the wrong word. But I did not interpreted the comment
as saying that the implementation was driven but a test program.
Ira
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Tue, Dec 08, 2015 at 08:28:17AM +0200, Leon Romanovsky wrote:
On Mon, Dec 07, 2015 at 03:43:10PM -0500, Dennis Dalessandro wrote:
+
+/*
+ * Things that are driver specific, module parameters in hfi1 and qib
+ */
+struct rvt_driver_params {
+ int max_pds;
Can it be negative value?
+};
If so no protection domains would ever get allocated. I don't think anything
else would break though if a driver were to do that.
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Tue, Dec 08, 2015 at 09:20:27AM +0200, Leon Romanovsky wrote:
On Tue, Dec 08, 2015 at 08:28:17AM +0200, Leon Romanovsky wrote:
On Mon, Dec 07, 2015 at 03:43:10PM -0500, Dennis Dalessandro wrote:
> +
> +/*
> + * Things that are driver specific, module parameters in hfi1 and qib
> + */
> +struct rvt_driver_params {
> + int max_pds;
Can it be negative value?
> +};
Forget my question, I see, you removed this variable in the following patch.
Well removed it from the rvt structure, but the concept still exists. I'm
now using struct ib_device_attr.max_pd.
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Tue, Dec 08, 2015 at 02:52:36PM -0500, ira. weiny wrote:
On Tue, Dec 08, 2015 at 01:17:48PM -0600, Sean Hefty wrote:
> > > +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> > > + struct ib_ucontext *context,
> > > + struct ib_udata *udata)
> > > +{
> > > +struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> > > +struct rvt_pd *pd;
> > > +struct ib_pd *ret;
> > > +
> > > +pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> > > +if (!pd) {
> > > +ret = ERR_PTR(-ENOMEM);
> > > +goto bail;
> > > +}
> > > +/*
> > > + * This is actually totally arbitrary. Some correctness tests
> > > + * assume there's a maximum number of PDs that can be allocated.
> > > + * We don't actually have this limit, but we fail the test if
> > > + * we allow allocations of more than we report for this value.
> > > + */
> >
> > Why not trap this in user space, rather than forcing the kernel to
> support some test program?
> >
>
> What do you mean "trap this in user space"? This code is not supporting
> any
> particular test program.
>
> If users try and allocate more protection domains then are advertised then
> an
> error should be returned.
>
> Perhaps the comment should be removed if it is confusing?
There is no theoretical limit on the number of PDs, or likely most other
resources. So why define and enforce an arbitrary limit? The justification
given was that some test program would fail. If there's a concern about that
test program passing, then enforce the limit in user space.
I did not interpret this as being driven by a test but rather by the IBTA spec.
This may be pedantic but, wouldn't it be confusing from a user POV to see an
advertised limit but then not be constrained by it? I'm not opposed to setting
this limit arbitrarily high, but I still think the implementation should
enforce that limit.
I didn't find the comment confusing. Just the choice to let a test app drive
the design.
Perhaps "confusing" is the wrong word. But I did not interpreted the comment
as saying that the implementation was driven but a test program.
How about I reword the comment to say that while we could continue
allocating PDs being only constrained by system resources, the spec says
there is a limit so we enforce that?
-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Thu, Dec 10, 2015 at 11:40:48AM -0500, Dennis Dalessandro wrote:
> On Tue, Dec 08, 2015 at 08:28:17AM +0200, Leon Romanovsky wrote:
> >On Mon, Dec 07, 2015 at 03:43:10PM -0500, Dennis Dalessandro wrote:
> >>+
> >>+/*
> >>+ * Things that are driver specific, module parameters in hfi1 and qib
> >>+ */
> >>+struct rvt_driver_params {
> >>+ int max_pds;
> >Can it be negative value?
> >>+};
>
> If so no protection domains would ever get allocated. I don't think anything
> else would break though if a driver were to do that.
In such way, it should be "unsigned int".
>
> -Denny
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
On Thu, Dec 10, 2015 at 11:49:40AM -0500, Dalessandro, Dennis wrote:
> On Tue, Dec 08, 2015 at 02:52:36PM -0500, ira. weiny wrote:
> >On Tue, Dec 08, 2015 at 01:17:48PM -0600, Sean Hefty wrote:
> >>> > > +struct ib_pd *rvt_alloc_pd(struct ib_device *ibdev,
> >>> > > +struct ib_ucontext *context,
> >>> > > +struct ib_udata *udata)
> >>> > > +{
> >>> > > + struct rvt_dev_info *dev = ib_to_rvt(ibdev);
> >>> > > + struct rvt_pd *pd;
> >>> > > + struct ib_pd *ret;
> >>> > > +
> >>> > > + pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> >>> > > + if (!pd) {
> >>> > > + ret = ERR_PTR(-ENOMEM);
> >>> > > + goto bail;
> >>> > > + }
> >>> > > + /*
> >>> > > + * This is actually totally arbitrary. Some correctness
> >>tests
> >>> > > + * assume there's a maximum number of PDs that can be
> >>allocated.
> >>> > > + * We don't actually have this limit, but we fail the test if
> >>> > > + * we allow allocations of more than we report for this
> >>value.
> >>> > > + */
> >>> >
> >>> > Why not trap this in user space, rather than forcing the kernel to
> >>> support some test program?
> >>> >
> >>>
> >>> What do you mean "trap this in user space"? This code is not supporting
> >>> any
> >>> particular test program.
> >>>
> >>> If users try and allocate more protection domains then are advertised
> >>then
> >>> an
> >>> error should be returned.
> >>>
> >>> Perhaps the comment should be removed if it is confusing?
> >>
> >>There is no theoretical limit on the number of PDs, or likely most other
> >>resources. So why define and enforce an arbitrary limit? The
> >>justification
> >>given was that some test program would fail. If there's a concern about
> >>that
> >>test program passing, then enforce the limit in user space.
> >
> >I did not interpret this as being driven by a test but rather by the IBTA
> >spec.
> >
> >This may be pedantic but, wouldn't it be confusing from a user POV to see
> >an
> >advertised limit but then not be constrained by it? I'm not opposed to
> >setting
> >this limit arbitrarily high, but I still think the implementation should
> >enforce that limit.
> >
> >>
> >>I didn't find the comment confusing. Just the choice to let a test app
> >>drive the design.
> >
> >Perhaps "confusing" is the wrong word. But I did not interpreted the
> >comment
> >as saying that the implementation was driven but a test program.
>
> How about I reword the comment to say that while we could continue
> allocating PDs being only constrained by system resources, the spec says
> there is a limit so we enforce that?
I'm ok with that, Sean?
Ira
>
> -Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 03/37] IB/rdmavt: Add protection domain to rdmavt.
> > How about I reword the comment to say that while we could continue > > allocating PDs being only constrained by system resources, the spec says > > there is a limit so we enforce that? > > I'm ok with that, Sean? That's fine -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
