Re: [PATCH 3/3 fixed] infiniband-diags: Add mkey support to config file

On Thu, 15 Mar 2012 11:23:02 -0700
Jim Foraker [off-list ref] wrote:

On Tue, 2012-03-13 at 05:29 -0700, Hal Rosenstock wrote:

quoted

On 3/9/2012 2:27 PM, Jim Foraker wrote:

quoted

On Fri, 2012-03-09 at 05:00 -0800, Hal Rosenstock wrote:

quoted

On 3/6/2012 5:18 PM, Jim Foraker wrote:

quoted

Support for "m_key=" parameter, 

Shouldn't mkey be on a per port basis rather than a single mkey for all
ports ?

     In the long run, I agree.  However, adding this support brings the
diags up to speed with the "one mkey per subnet" model that OpenSM
currently (sort of) supports.  

OpenSM mkey support is not complete for even that model. IMO the diags
should not just be in sync with the current OpenSM (a minimum bar for
this support) but support the general requirement. In the end, I suspect
there will be several mkey policies supported by OpenSM.

     I've got some tentative OpenSM patches which I'll hopefully be
ready to post once we've hashed out the tools side.  At the moment,
they're centered around the "one mkey to rule them all" model too, but
should make it easier to support other model(s).

quoted

quoted

We should do better down the road, but I
don't think this patch gets in the way of that.

I wouldn't think the approach for a per port MKey would use a single
MKey parameter and store it in the source port structure.

     I was referring specifically to this patch, and in particular the
visible change, adding a "m_key" option to the config file.
     Dumping the mkey in the source port structure is absolutely a hack.
The problem is that no real context is kept on the destination port in
the current API, which near as I can tell means supporting mkeys
(particularly multiple mkeys) cleanly requires either bumping the API
version, or falling back on the deprecated saving of global state in the
library.  This would need to be cleaned up, but we could bunch that
change with others as part of a future API rev.

Actually this is "solved" via the current patch.  While "storing" the mkey in
the source port structure is less than ideal.  Using this method allows the
source port to become a context for the other calls.  Using the
smp_mkey_set/get functions allow you to change the mkey for each call without
disturbing the ABI.

That said there are ways to make this right but they involve massive changes
to this library and the diags.  I have made attempts in the past to move
things in this direction and have been met with resistance.  Until I come up
with a real solution I suggest we not belabour that point any more.

At this point for better or worse this is the interface we have.  I don't want
to bump the ABI unless _absolutely_ necessary.

Ira

quoted

-- Hal

quoted

quoted

quoted

plus config file now installs user-read-only.

This part is best as a separate patch.

     I agree.  I was worried that applying the config change without the
permissions change may open a potential vulnerability, but the answer
there is just to have the permissions change earlier in the tree than
the new parameter.

     Jim

quoted

-- Hal

quoted

Signed-off-by: Jim Foraker <redacted>
---
 Makefile.am         |    2 +-
 etc/ibdiag.conf     |    2 ++
 src/ibdiag_common.c |    2 ++
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 950f95b..ef59bd2 100644
--- a/Makefile.am
+++ b/Makefile.am

@@ -112,4 +112,4 @@ install-data-hook:
 	fi
 	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/scripts/IBswcountlimits.pm $(DESTDIR)/$(PERL_INSTALLDIR)/IBswcountlimits.pm
 	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/etc/error_thresholds $(DESTDIR)/$(sysconfdir)/infiniband-diags
-	$(top_srcdir)/config/install-sh -c -m 444 $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags
+	$(top_srcdir)/config/install-sh -c -m 400 $(top_srcdir)/etc/ibdiag.conf $(DESTDIR)/$(sysconfdir)/infiniband-diags

diff --git a/etc/ibdiag.conf b/etc/ibdiag.conf
index 77f3ce9..2a2334f 100644
--- a/etc/ibdiag.conf
+++ b/etc/ibdiag.conf

@@ -15,3 +15,5 @@
 # Default = true
 #MLX_EPI=false
 
+# define a default m_key
+#m_key=0x00

diff --git a/src/ibdiag_common.c b/src/ibdiag_common.c
index 0901231..2089847 100644
--- a/src/ibdiag_common.c
+++ b/src/ibdiag_common.c

@@ -155,6 +155,8 @@ void read_ibdiag_config(const char *file)
 			} else {
 				ibd_ibnetdisc_flags &= ~IBND_CONFIG_MLX_EPI;
 			}
+		} else if (strncmp(name, "m_key", strlen("m_key")) == 0) {
+			ibd_mkey = strtoull(val_str, 0, 0);
 		}
 	}
 

--
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

--
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

-- 
Ira Weiny
Member of Technical Staff
Lawrence Livermore National Lab
925-423-8008
weiny2-i2BcT+NCU+M@public.gmane.org
--
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