Re: [PATCH] infiniband-diags: add rdma-ndd daemon
On 11/10/2014 1:32 PM, Weiny, Ira wrote: > I think changing the default is a worthwhile change. In addition, alternate > admin policies are aided by the > general use of the %h specifier. > > 1) SM's which periodically scan the Node Description always get up to > date hostname info. > 2) Up to date hostname info is provided even if a user space daemons > fails. > Note: OpenSM has an update node description feature for this > condition. > 3) Low level diag tools always get up to date hostname info There is a node description local changes trap which should cause an SM to reread the updated NodeDescription. This is assuming the "SMA" is compliant and issues such trap when ND changes and this occurs after LinkUp. ALso, there is a new optional (SMA) feature (@ 1.3) to set the NodeDescription. Not sure if this helps for this. > Do you believe that " " is an unreasonable default? > > Roland, Hal, do you have any input? I was concerned with having the format be self identifying so that it can be easily distinguishable from other known formats being used in the field. As you wrote, given that RedHat is already using this format, we are already "living" with this. -- Hal -- 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] infiniband-diags: add rdma-ndd daemon
On Mon, Nov 10, 2014 at 06:32:18PM +, Weiny, Ira wrote: > > I don't know if this is practical, but it is the only race free way to > > properly > > address all of this. > > I don't think it is practical at all. > > Assuming that we can hold the link from transitioning to Init how PortInfo writes can do that > does any userspace software know if the current hostname is "valid"? > If, at the time of module load, the hostname is "localhost" you end > up with the same "problem" as the proposed kernel patches. Deciding when to turn on the IB interface in the boot sequence is a policy issue for userspace. Some setups will require this to be done in the initrd (ie boot over IB), others will want to do this after management networking comes up (and the host name is set). Once rdma-ndd starts running the hostname will track just the same as if there was kernel %h support, and having your init sequence the IB interface up after you have the hostname ensures it isn't local host. This sequencing creates the opportunity for the admin to actually set the host name to something that is not the kernel default. > I'm not against adding this to rdma-ndd but I would like it to be > optional. With the kernel patches rdma-ndd itself is optional. Not really, without rdma-ndd the node description is able to change on its own and no traps are generated. That seems pretty broken. So you need to run rdma-ndd to have IBA compliance. > I think changing the default is a worthwhile change. In addition, > alternate admin policies are aided by the general use of the %h > specifier. I think you need to resolve this split responsibility somehow. The kernel needs to work correctly without the daemon. To me that either means it never changes the node desc on its own (what we have today), or it always generates a trap when it changes. > Do you believe that " " is an unreasonable default? No, but what if I want it to say "OSD " on some systems - then what? How do I set that and avoid generating traps at boot? 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] infiniband-diags: add rdma-ndd daemon
> > On Mon, Nov 10, 2014 at 12:28:27AM +, Weiny, Ira wrote: > > > > > > Sadly, I think the proper way to address this is the same way netdev > > > addresses it - do not activate the interface on module load, wait > > > for an explicit enablement so userspace can configure before it tries to > > > link > up. > > > > > > (Not to say that is even possible considering RDMA's history, but > > > still..) > > > > I don't think this is really possible. When would you consider the > > "interface" to be "up"? Even before the port goes active the SM and > > other diags can query this value. This is why I propose that the > > default of the drivers, without user space involvement, should change. > > The user space daemon is only trying to react to other user space > > activity. > > Same as net dev, after the module load the physical layer is disabled. Nothing > can see the NodeDescription because the link is kept down. > > The everything gets configured, then the physical layer is allowed to come up. > > I don't know if this is practical, but it is the only race free way to > properly > address all of this. I don't think it is practical at all. Assuming that we can hold the link from transitioning to Init how does any userspace software know if the current hostname is "valid"? If, at the time of module load, the hostname is "localhost" you end up with the same "problem" as the proposed kernel patches. In fact the proposed kernel patches actually aid in the setting of alternate policies in that a %h provides some dynamic lookup. (See below) > > > > TBH, I'd rather see just this daemon and drop the kernel side... If > > > the daemon is started before the modules are loaded then it might be > > > able to set the description while the port is still down. > > > > The problem is this sequence. > > > > 1) rdma-ndd daemon started (hostname == localhost, no rdma devices > > set) > > 2) dhcp started (or other hostname change, rdma-ndd triggered but no > > rdma devices set; not loaded yet) > > 3) rdma devices loaded, default node_desc > > 4) daemon detects new rdma device instantly and programs the admin >desired node name in the 100ms before the physical link reaches >up. Since the link is down when the change occurs no traps are scheduled. >Or in the 10% case where the race is lost you get a trap. Oh well. I'm not against adding this to rdma-ndd but I would like it to be optional. With the kernel patches rdma-ndd itself is optional. > > > What I have proposed will work no matter what order things are > > started/loaded. > > Well, not really - it works in the sense that the kernel default node name > works > properly, but it doesn't allow the admin to set a custom name without hitting > all these same issues (well, except perhaps by module parameter..). I think changing the default is a worthwhile change. In addition, alternate admin policies are aided by the general use of the %h specifier. 1) SM's which periodically scan the Node Description always get up to date hostname info. 2) Up to date hostname info is provided even if a user space daemons fails. Note: OpenSM has an update node description feature for this condition. 3) Low level diag tools always get up to date hostname info Do you believe that " " is an unreasonable default? Roland, Hal, do you have any input? > > > Finally, I contemplated the alternative of having the rdma-ndd daemon > > detect new rdma devices. > > Well, without that all it does is link two parts of the kernel together > through > user space with no way toset policy in userspace (policy in this case is the > node > description string). Seems very strange. Yes the daemon is somewhat strange and I envision it being updated for other policies in the future which may include some of the functionality you suggest. But it seemed appropriate to add it now as it enhances the SM knowing about hostname changes. -- 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] infiniband-diags: add rdma-ndd daemon
On Mon, Nov 10, 2014 at 12:28:27AM +, Weiny, Ira wrote: > > > > Sadly, I think the proper way to address this is the same way netdev > > addresses > > it - do not activate the interface on module load, wait for an explicit > > enablement so userspace can configure before it tries to link up. > > > > (Not to say that is even possible considering RDMA's history, but still..) > > I don't think this is really possible. When would you consider the > "interface" to be "up"? Even before the port goes active the SM and > other diags can query this value. This is why I propose that the > default of the drivers, without user space involvement, should > change. The user space daemon is only trying to react to other user > space activity. Same as net dev, after the module load the physical layer is disabled. Nothing can see the NodeDescription because the link is kept down. The everything gets configured, then the physical layer is allowed to come up. I don't know if this is practical, but it is the only race free way to properly address all of this. > > TBH, I'd rather see just this daemon and drop the kernel side... If the > > daemon is > > started before the modules are loaded then it might be able to set the > > description while the port is still down. > > The problem is this sequence. > > 1) rdma-ndd daemon started (hostname == localhost, no rdma devices set) > 2) dhcp started (or other hostname change, rdma-ndd triggered but no rdma > devices set; not loaded yet) > 3) rdma devices loaded, default node_desc 4) daemon detects new rdma device instantly and programs the admin desired node name in the 100ms before the physical link reaches up. Since the link is down when the change occurs no traps are scheduled. Or in the 10% case where the race is lost you get a trap. Oh well. > What I have proposed will work no matter what order things are > started/loaded. Well, not really - it works in the sense that the kernel default node name works properly, but it doesn't allow the admin to set a custom name without hitting all these same issues (well, except perhaps by module parameter..). > Finally, I contemplated the alternative of having the rdma-ndd > daemon detect new rdma devices. Well, without that all it does is link two parts of the kernel together through user space with no way toset policy in userspace (policy in this case is the node description string). Seems very strange. 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] infiniband-diags: add rdma-ndd daemon
> > Sadly, I think the proper way to address this is the same way netdev addresses > it - do not activate the interface on module load, wait for an explicit > enablement so userspace can configure before it tries to link up. > > (Not to say that is even possible considering RDMA's history, but still..) I don't think this is really possible. When would you consider the "interface" to be "up"? Even before the port goes active the SM and other diags can query this value. This is why I propose that the default of the drivers, without user space involvement, should change. The user space daemon is only trying to react to other user space activity. > > TBH, I'd rather see just this daemon and drop the kernel side... If the > daemon is > started before the modules are loaded then it might be able to set the > description while the port is still down. The problem is this sequence. 1) rdma-ndd daemon started (hostname == localhost, no rdma devices set) 2) dhcp started (or other hostname change, rdma-ndd triggered but no rdma devices set; not loaded yet) 3) rdma devices loaded, default node_desc What I have proposed will work no matter what order things are started/loaded. Furthermore, it avoids races between the start up scripts and the SM because the current hostname is always mapped at the time the query is being answered. With parallel start up systems like systemd you could have the hostname set by DHCP while some of your rdma devices are loaded and others are not. Those which were loaded before the hostname change will get set by rdma-ndd and trigger a trap. The others will simply pick up the correct hostname in the initial SM query. Finally, I contemplated the alternative of having the rdma-ndd daemon detect new rdma devices. I don't like this solution because it results in additional unnecessary MAD traffic for those devices. (ie. initial ND query, followed by a trap/trap repress, and the final "correct" ND query.) With my proposed solution any devices loaded after the hostname change (or in the case of a static hostname) will be properly queried with one ND query. -- 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] infiniband-diags: add rdma-ndd daemon
On Thu, Oct 30, 2014 at 07:54:30PM -0400, ira.we...@intel.com wrote: > From: Ira Weiny > > rdma-ndd is a system daemon which watches the procfs hostname file for > updates. > > Upon detecting an update it will update the Node Descriptions of the RDMA > devices in the system. > > This deamon is intended to work with kernels which support polling of the > procfs hostname file. If your kernel does not support this feature the daemon > will set the Node Descriptions to the hostname at start up and then sleep > forever. It seems so strange to have kernel processing of %h/etc and then to turn around and use a user space daemon to connect one bit of the kernel to another Sadly, I think the proper way to address this is the same way netdev addresses it - do not activate the interface on module load, wait for an explicit enablement so userspace can configure before it tries to link up. (Not to say that is even possible considering RDMA's history, but still..) TBH, I'd rather see just this daemon and drop the kernel side... If the daemon is started before the modules are loaded then it might be able to set the description while the port is still down. 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] infiniband-diags: add rdma-ndd daemon
>
> > How about the following?
> >
> > @@ -155,8 +156,8 @@ int read_and_set_hostname(int fd)
> > {
> > int rc;
> > char buf[128];
> > - if (read(fd, buf, 65) >= 0) {
> > - buf[65] = '\0';
> > + memset(buf, 0, sizeof(buf));
> > + if (read(fd, buf, 64) >= 0) {
> > newline_to_null(buf);
> > strip_domain(buf);
> > syslog(LOG_INFO, "new hostname detected: %s", buf);
>
> This change looks fine to me. A possible alternative is to use dup() +
> fdopen() + fgets() + fclose(). fgets() namely automatically appends a
> terminating '\0'.
Agreed. But I'd like to avoid creating additional fd's.
Thanks for the review,
Ira
>
> Bart.
>
> --
> 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] infiniband-diags: add rdma-ndd daemon
On 10/31/14 10:24, Weiny, Ira wrote:
On 10/31/14 00:54, ira.we...@intel.com wrote:
> +int read_and_set_hostname(int fd)
> +{
> +int rc;
> +char buf[128];
> +if (read(fd, buf, 65) >= 0) {
> +buf[65] = '\0';
> +newline_to_null(buf);
> +strip_domain(buf);
> +syslog(LOG_INFO, "new hostname detected: %s", buf);
> +set_rdma_device_names((char *)buf);
> +rc = 0;
> +} else {
> +syslog(LOG_ERR, "Read %s Failed\n", SYS_HOSTNAME);
> +rc = -EIO;
> +}
> +return rc;
> +}
The above code will trigger reading uninitialized data if the data read from fd
is not newline-terminated and less than 65 characters are read.
I'm not sure what you mean. Do you mean "will trigger _using_ uninitialized
data"?
Yes.
How about the following?
@@ -155,8 +156,8 @@ int read_and_set_hostname(int fd)
{
int rc;
char buf[128];
- if (read(fd, buf, 65) >= 0) {
- buf[65] = '\0';
+ memset(buf, 0, sizeof(buf));
+ if (read(fd, buf, 64) >= 0) {
newline_to_null(buf);
strip_domain(buf);
syslog(LOG_INFO, "new hostname detected: %s", buf);
This change looks fine to me. A possible alternative is to use dup() +
fdopen() + fgets() + fclose(). fgets() namely automatically appends a
terminating '\0'.
Bart.
--
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] infiniband-diags: add rdma-ndd daemon
>
> On 10/31/14 00:54, ira.we...@intel.com wrote:
> > +int set_rdma_device_names(const char *hostname) {
> > + DIR *class_dir;
> > + struct dirent *dent;
> > +
> > + class_dir = opendir(SYS_INFINIBAND);
> > + if (!class_dir) {
> > + syslog(LOG_INFO, "Failed to open %s", SYS_INFINIBAND);
> > + return -ENOSYS;
> > + }
> > +
> > + while ((dent = readdir(class_dir))) {
> > + int retry = set_retry_cnt;
> > + if (dent->d_name[0] == '.')
> > + continue;
> > +
> > + while (update_node_desc(dent->d_name, hostname) && retry
> > 0) {
> > + syslog(LOG_ERR, "retrying set Node Description on
> %s\n",
> > + dent->d_name);
> > + retry--;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> Has this code been tested with Valgrind and --track-fds=yes and --leak-
> check=full ? I see an opendir() call in the above code but no closedir().
Great catch thanks!
I'll run with valgrind on v2.
>
> > +int read_and_set_hostname(int fd)
> > +{
> > + int rc;
> > + char buf[128];
> > + if (read(fd, buf, 65) >= 0) {
> > + buf[65] = '\0';
> > + newline_to_null(buf);
> > + strip_domain(buf);
> > + syslog(LOG_INFO, "new hostname detected: %s", buf);
> > + set_rdma_device_names((char *)buf);
> > + rc = 0;
> > + } else {
> > + syslog(LOG_ERR, "Read %s Failed\n", SYS_HOSTNAME);
> > + rc = -EIO;
> > + }
> > + return rc;
> > +}
>
> The above code will trigger reading uninitialized data if the data read from
> fd
> is not newline-terminated and less than 65 characters are read.
I'm not sure what you mean. Do you mean "will trigger _using_ uninitialized
data"?
How about the following?
@@ -155,8 +156,8 @@ int read_and_set_hostname(int fd)
{
int rc;
char buf[128];
- if (read(fd, buf, 65) >= 0) {
- buf[65] = '\0';
+ memset(buf, 0, sizeof(buf));
+ if (read(fd, buf, 64) >= 0) {
newline_to_null(buf);
strip_domain(buf);
syslog(LOG_INFO, "new hostname detected: %s", buf);
Ira
>
> Bart.
> --
> 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] infiniband-diags: add rdma-ndd daemon
On 10/31/14 00:54, ira.we...@intel.com wrote:
+int set_rdma_device_names(const char *hostname)
+{
+ DIR *class_dir;
+ struct dirent *dent;
+
+ class_dir = opendir(SYS_INFINIBAND);
+ if (!class_dir) {
+ syslog(LOG_INFO, "Failed to open %s", SYS_INFINIBAND);
+ return -ENOSYS;
+ }
+
+ while ((dent = readdir(class_dir))) {
+ int retry = set_retry_cnt;
+ if (dent->d_name[0] == '.')
+ continue;
+
+ while (update_node_desc(dent->d_name, hostname) && retry > 0) {
+ syslog(LOG_ERR, "retrying set Node Description on %s\n",
+ dent->d_name);
+ retry--;
+ }
+ }
+
+ return 0;
+}
Has this code been tested with Valgrind and --track-fds=yes and
--leak-check=full ? I see an opendir() call in the above code but no
closedir().
> +int read_and_set_hostname(int fd)
> +{
> + int rc;
> + char buf[128];
> + if (read(fd, buf, 65) >= 0) {
> + buf[65] = '\0';
> + newline_to_null(buf);
> + strip_domain(buf);
> + syslog(LOG_INFO, "new hostname detected: %s", buf);
> + set_rdma_device_names((char *)buf);
> + rc = 0;
> + } else {
> + syslog(LOG_ERR, "Read %s Failed\n", SYS_HOSTNAME);
> + rc = -EIO;
> + }
> + return rc;
> +}
The above code will trigger reading uninitialized data if the data read
from fd is not newline-terminated and less than 65 characters are read.
Bart.
--
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
