RE: [PATCH 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On Mon, Jan 19, 2015 at 10:50:38PM +, Weiny, Ira wrote:
On Mon, Jan 19, 2015 at 07:57:22PM +, Weiny, Ira wrote:
Perhaps you can give us an example of where the current code would
work without modifications if the IBTA were to define any new base
or smp class version?
I think the point here is that every check for OPA_MIN_CLASS_VERSION
must also be combined with a check if the device in question is OPA or
not.
This contradicts what you say below
How so?
if (mad_reg_req-mgmt_class_version = OPA_MIN_CLASS_VERSION
!port_priv-qp_info[qpn].supports_jumbo_mads) {
Sorry, I got a bit confused because this is not the latest version of the patch.
Should be:
if (port_is_opa mad_reg_req-mgmt_class_version =
OPA_MIN_CLASS_VERSION
!port_priv-qp_info[qpn].supports_jumbo_mads) {
And since port_is_opa = true implies
port_priv-qp_info[qpn].supports_jumbo_mads = true the above if can never
evaluate to true and can just be removed.
Right there is no need to check for supports_jumbo_mads. The latest version
was:
+ if (mad_reg_req-mgmt_class_version = OPA_MIN_CLASS_VERSION
+ !(port_priv-device-attributes.device_cap_flags2
IB_DEVICE_OPA_MAD_SUPPORT)) {
The bug in the patch is that any class version should be allowed on IB
devices. This check prevented class versions = 0x80 for IB devices.
NOTE however that the current code already limits users to a class version 8.
(due to the implementation.)
-#define MAX_MGMT_VERSION 8
+#define MAX_MGMT_VERSION 0x83
...
if (mad_reg_req-mgmt_class_version = MAX_MGMT_VERSION) {
dev_notice(device-dev,
ib_register_mad_agent: invalid Class
Version %u\n,
mad_reg_req-mgmt_class_version);
goto error1;
}
Therefore I did not see this as limiting the IB implementation we already have.
I will remove the patch as I agree with Jason and Hal it is technically more
correct.
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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On Mon, Jan 19, 2015 at 10:50:38PM +, Weiny, Ira wrote:
On Mon, Jan 19, 2015 at 07:57:22PM +, Weiny, Ira wrote:
Perhaps you can give us an example of where the current code would
work without modifications if the IBTA were to define any new base or
smp class version?
I think the point here is that every check for OPA_MIN_CLASS_VERSION must
also be combined with a check if the device in question is OPA or not.
This contradicts what you say below
How so?
if (mad_reg_req-mgmt_class_version = OPA_MIN_CLASS_VERSION
!port_priv-qp_info[qpn].supports_jumbo_mads) {
Should be:
if (port_is_opa mad_reg_req-mgmt_class_version = OPA_MIN_CLASS_VERSION
!port_priv-qp_info[qpn].supports_jumbo_mads) {
And since port_is_opa = true implies
port_priv-qp_info[qpn].supports_jumbo_mads = true the above if can
never evaluate to true and can just be removed.
Jason
--
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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On Mon, Jan 19, 2015 at 07:57:22PM +, Weiny, Ira wrote:
Perhaps you can give us an example of where the current code would
work without modifications if the IBTA were to define any new base or
smp class version?
I think the point here is that every check for OPA_MIN_CLASS_VERSION must
also be combined with a check if the device in question is OPA or not.
This contradicts what you say below
I don't see a problem with sharing the kernel structs or MAD processing, as
long as all OPA specific behavior is conditionalized on the device in question
being OPA - it should never be conditionalized on the class version of the
MAD.
commit a70a5af286b2985a0a95e9733d1e3166845a8be8
Author: Ira Weiny ira.we...@intel.com
Date: Tue Sep 23 20:04:49 2014 -0400
IB/mad: Add registration check for Intel Omni-Path Architecture
MADs
For instance:
+ if (mad_reg_req-mgmt_class_version =
OPA_MIN_CLASS_VERSION
+ !port_priv-qp_info[qpn].supports_jumbo_mads) {
Is wrong because it means that no IB software can register for those MADs
anymore, which is cross polluting the two architectures in the kernel.
Presumably since supports_jumbo_mads is always true for OPA the above test
is not required at all and this patch should be dropped.
But I expect if we keep looking at uses of OPA_MIN_CLASS_VERSION we will
see other cases where they need to be conditionalized?
That is the only use of OPA_MIN_CLASS_VERSION. I agree with your second
assessment and I'll remove the patch from the next series.
Hal was this your assessment as well?
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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On Mon, Jan 19, 2015 at 07:57:22PM +, Weiny, Ira wrote:
Perhaps you can give us an example of where the current code would
work without modifications if the IBTA were to define any new base
or smp class version?
I think the point here is that every check for OPA_MIN_CLASS_VERSION
must also be combined with a check if the device in question is OPA or
not.
I don't see a problem with sharing the kernel structs or MAD
processing, as long as all OPA specific behavior is conditionalized on
the device in question being OPA - it should never be conditionalized
on the class version of the MAD.
commit a70a5af286b2985a0a95e9733d1e3166845a8be8
Author: Ira Weiny ira.we...@intel.com
Date: Tue Sep 23 20:04:49 2014 -0400
IB/mad: Add registration check for Intel Omni-Path Architecture MADs
For instance:
+ if (mad_reg_req-mgmt_class_version = OPA_MIN_CLASS_VERSION
+!port_priv-qp_info[qpn].supports_jumbo_mads) {
Is wrong because it means that no IB software can register for those
MADs anymore, which is cross polluting the two architectures in the
kernel.
Presumably since supports_jumbo_mads is always true for OPA the above
test is not required at all and this patch should be dropped.
But I expect if we keep looking at uses of OPA_MIN_CLASS_VERSION we
will see other cases where they need to be conditionalized?
Jason
--
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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/16/2015 2:49 PM, Weiny, Ira wrote: On 1/15/2015 6:30 PM, Weiny, Ira wrote: On 1/12/2015 12:11 PM, ira.we...@intel.com wrote: From: Ira Weiny ira.we...@intel.com OPA_MIN_CLASS_VERSION -- OPA Class versions are 0x80 OPA_SMP_CLASS_VERSION -- Defined at 0x80 OPA_MGMT_BASE_VERSION -- Defined at 0x80 Increase max management version to accommodate OPA Allocation of MAD base and class version numbers is owned by the IBTA. It doesn't seem appropriate to arbitrarily claim code points without proper approval. OPA is its own architecture space. While this space uses some of the same values as IB we are not claiming any IBTA values. You *are* claiming IBTA values. When the IBTA chooses to use those values, then there will be a conflict. There is no conflict. It is true that when the IBTA assigns meaning to those values the code may have to be changed to interpret this new meaning. That has to happen regardless of these patches or their meaning on OPA devices. Currently, these patches are claiming values that are controlled by an established standards organization. It is the equivalent of just grabbing a currently unused Ethertype value for a new protocol and then claiming that whenever the IEEE allocates it for something else you will make changes to resolve the conflict. There are then a range of kernel versions which would do the wrong thing with the new IEEE value rather than indicate that it is not (currently) supported. It's not an appropriate practice in the design of networking protocols. -- Hal 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
Currently, these patches are claiming values that are controlled by an established standards organization. It is the equivalent of just grabbing a currently unused Ethertype value for a new protocol and then claiming that whenever the IEEE allocates it for something else you will make changes to resolve the conflict. It would be equivalent if OPA was Infiniband, but it is not. It is a separate architecture, with an entirely separate name space. Conceptually, it is similar to the IB GRH and the IPv6 header. They have a similar structure, but they are not the same. The IBTA can define a new GRH field values, such as NxtHdr, without consulting or getting approval from the IETF. Ira could introduce patches to define OPA management packets, but the general structure of those packet headers would be exactly the same as IB management packets. There's no need to introduce duplicate definitions into the code base. The existing structures can be reused. However, OPA makes use of different values for the fields. Perhaps you can give us an example of where the current code would work without modifications if the IBTA were to define any new base or smp class version? Would removing this patch satisfy your concerns? commit a70a5af286b2985a0a95e9733d1e3166845a8be8 Author: Ira Weiny ira.we...@intel.com Date: Tue Sep 23 20:04:49 2014 -0400 IB/mad: Add registration check for Intel Omni-Path Architecture MADs If the registration specifies an OPA MAD class version and the device does not support OPA MADs, fail the MAD registration. Signed-off-by: Ira Weiny ira.we...@intel.com 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
Currently, these patches are claiming values that are controlled by an established standards organization. It is the equivalent of just grabbing a currently unused Ethertype value for a new protocol and then claiming that whenever the IEEE allocates it for something else you will make changes to resolve the conflict. It would be equivalent if OPA was Infiniband, but it is not. It is a separate architecture, with an entirely separate name space. Conceptually, it is similar to the IB GRH and the IPv6 header. They have a similar structure, but they are not the same. The IBTA can define a new GRH field values, such as NxtHdr, without consulting or getting approval from the IETF. Ira could introduce patches to define OPA management packets, but the general structure of those packet headers would be exactly the same as IB management packets. There's no need to introduce duplicate definitions into the code base. The existing structures can be reused. However, OPA makes use of different values for the fields. - Sean -- 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/19/2015 1:28 PM, Hefty, Sean wrote: Currently, these patches are claiming values that are controlled by an established standards organization. It is the equivalent of just grabbing a currently unused Ethertype value for a new protocol and then claiming that whenever the IEEE allocates it for something else you will make changes to resolve the conflict. It would be equivalent if OPA was Infiniband, but it is not. It is a separate architecture, with an entirely separate name space. But the OPA MAD patches currently share the IBA name space. -- Hal Conceptually, it is similar to the IB GRH and the IPv6 header. They have a similar structure, but they are not the same. The IBTA can define a new GRH field values, such as NxtHdr, without consulting or getting approval from the IETF. Ira could introduce patches to define OPA management packets, but the general structure of those packet headers would be exactly the same as IB management packets. There's no need to introduce duplicate definitions into the code base. The existing structures can be reused. However, OPA makes use of different values for the fields. - Sean -- 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/15/2015 6:30 PM, Weiny, Ira wrote: On 1/12/2015 12:11 PM, ira.we...@intel.com wrote: From: Ira Weiny ira.we...@intel.com OPA_MIN_CLASS_VERSION -- OPA Class versions are 0x80 OPA_SMP_CLASS_VERSION -- Defined at 0x80 OPA_MGMT_BASE_VERSION -- Defined at 0x80 Increase max management version to accommodate OPA Allocation of MAD base and class version numbers is owned by the IBTA. It doesn't seem appropriate to arbitrarily claim code points without proper approval. OPA is its own architecture space. While this space uses some of the same values as IB we are not claiming any IBTA values. You *are* claiming IBTA values. When the IBTA chooses to use those values, then there will be a conflict. -- Hal This is similar to how IB GIDs and IPv6 addresses look the same but are in separate addressing domains. 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/15/2015 6:30 PM, Weiny, Ira wrote: On 1/12/2015 12:11 PM, ira.we...@intel.com wrote: From: Ira Weiny ira.we...@intel.com OPA_MIN_CLASS_VERSION -- OPA Class versions are 0x80 OPA_SMP_CLASS_VERSION -- Defined at 0x80 OPA_MGMT_BASE_VERSION -- Defined at 0x80 Increase max management version to accommodate OPA Allocation of MAD base and class version numbers is owned by the IBTA. It doesn't seem appropriate to arbitrarily claim code points without proper approval. OPA is its own architecture space. While this space uses some of the same values as IB we are not claiming any IBTA values. You *are* claiming IBTA values. When the IBTA chooses to use those values, then there will be a conflict. There is no conflict. It is true that when the IBTA assigns meaning to those values the code may have to be changed to interpret this new meaning. That has to happen regardless of these patches or their meaning on OPA devices. 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/12/2015 12:11 PM, ira.we...@intel.com wrote: From: Ira Weiny ira.we...@intel.com OPA_MIN_CLASS_VERSION -- OPA Class versions are 0x80 OPA_SMP_CLASS_VERSION -- Defined at 0x80 OPA_MGMT_BASE_VERSION -- Defined at 0x80 Increase max management version to accommodate OPA Allocation of MAD base and class version numbers is owned by the IBTA. It doesn't seem appropriate to arbitrarily claim code points without proper approval. OPA is its own architecture space. While this space uses some of the same values as IB we are not claiming any IBTA values. This is similar to how IB GIDs and IPv6 addresses look the same but are in separate addressing domains. 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 16/20] IB/mad: Add Intel Omni-Path Architecture defines
On 1/12/2015 12:11 PM, ira.we...@intel.com wrote:
From: Ira Weiny ira.we...@intel.com
OPA_MIN_CLASS_VERSION -- OPA Class versions are 0x80
OPA_SMP_CLASS_VERSION -- Defined at 0x80
OPA_MGMT_BASE_VERSION -- Defined at 0x80
Increase max management version to accommodate OPA
Allocation of MAD base and class version numbers is owned by the IBTA.
It doesn’t seem appropriate to arbitrarily claim code points without
proper approval.
-- Hal
Signed-off-by: Ira Weiny ira.we...@intel.com
---
drivers/infiniband/core/mad_priv.h |4 +++-
include/rdma/ib_mad.h |5 -
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/core/mad_priv.h
b/drivers/infiniband/core/mad_priv.h
index d71ddcc..a7041b0 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -56,11 +56,13 @@
/* Registration table sizes */
#define MAX_MGMT_CLASS 80
-#define MAX_MGMT_VERSION 8
+#define MAX_MGMT_VERSION 0x83
#define MAX_MGMT_OUI 8
#define MAX_MGMT_VENDOR_RANGE2 (IB_MGMT_CLASS_VENDOR_RANGE2_END - \
IB_MGMT_CLASS_VENDOR_RANGE2_START + 1)
+#define OPA_MIN_CLASS_VERSION0x80
+
struct ib_mad_list_head {
struct list_head list;
struct ib_mad_queue *mad_queue;
diff --git a/include/rdma/ib_mad.h b/include/rdma/ib_mad.h
index 80e7cf4..8938f1e 100644
--- a/include/rdma/ib_mad.h
+++ b/include/rdma/ib_mad.h
@@ -42,8 +42,11 @@
#include rdma/ib_verbs.h
#include uapi/rdma/ib_user_mad.h
-/* Management base version */
+/* Management base versions */
#define IB_MGMT_BASE_VERSION 1
+#define OPA_MGMT_BASE_VERSION0x80
+
+#define OPA_SMP_CLASS_VERSION0x80
/* Management classes */
#define IB_MGMT_CLASS_SUBN_LID_ROUTED0x01
--
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
