| Message ID | 1371738080-18537-1-git-send-email-jsquyres@cisco.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 06/20/2013 10:21 AM, Jeff Squyres wrote: > Keep IBV_MTU_* enums values as they are, but pass MTU values around as > int's. This is an ABI-compatible change; legacy applications will use > the enum values, I'm not really concerned with what the legacy apps use so much as what they are presented with. In other words, I'm sure they won't request anything other than an MTU represented by one of the enum values. The problem though, is what if they are run on a link with a non-IB MTU and they are presented with it? Let's look at one of the ports you did in this patch as an example: > diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c > index 21c551d..5a0656f 100644 > --- a/examples/ud_pingpong.c > +++ b/examples/ud_pingpong.c > @@ -332,7 +332,7 @@ static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int size, > fprintf(stderr, "Unable to query port info for port %d\n", port); > goto clean_device; > } > - mtu = 1 << (port_info.active_mtu + 7); > + mtu = ibv_mtu_to_num(port_info.active_mtu); > if (size > mtu) { > fprintf(stderr, "Requested size larger than port MTU (%d)\n", mtu); > goto clean_device; That used to be a valid mathematical construct. Now it isn't. It will work for all of the IBV_MTU_* values, but if you run this program, unmodified, on an MTU 9000 link, you get 1 << 9007 ;-) Saying that this is backwards compatible is therefore incorrect. If it were entirely backwards compatible, then apps would not need to be recompiled in order to avoid this error. One possible solution to this problem is to use ld.linux's symbolic symbol versions to solve this problem for us. Fortunately, Roland has been excellent in the past about keeping all of libibverbs symbols versioned. That can save us here. We would need to redefine the active_mtu and max_mtu in ibv_device_attr and path_mtu in ibv_qp_attr to all be ints. No need to maintain back compatibility. Then we define ibv_device_attr_1.1 and ibv_qp_attr_1.1 structs that use the old enum. Then we define version IBVERBS_1.2 version of ibv_get_device_list plus a version 1.2 of any other symbols that pass around ibv_device_attr struct and ibv_qp_attr struct. The new default will be to use the new structs that have MTUs defined as ints, but the old 1.1 version of things will use the compat structs to pass things around. When we query the kernel about a device/qp, if we are linked against an old app we will be using the compat struct and we can do the conversion from int MTU to ibv_enum based MTU and vice versa in the IBVERBS_1.1 wrapper functions that need to be called to convert ints to enums and vice versa. It's a lot more work, but it's the right way to do this. So, sorry Jeff, but I'm going to Nack this patch as it stands on design and back compatibility. This really needs an API update, and it can be done in a back compatible way by using the shared library symbol version mapping and compat wrapper functions. -- 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
On 06/20/2013 12:34 PM, Doug Ledford wrote: > On 06/20/2013 10:21 AM, Jeff Squyres wrote: >> Keep IBV_MTU_* enums values as they are, but pass MTU values around as >> int's. This is an ABI-compatible change; legacy applications will use >> the enum values, > > I'm not really concerned with what the legacy apps use so much as what > they are presented with. In other words, I'm sure they won't request > anything other than an MTU represented by one of the enum values. The > problem though, is what if they are run on a link with a non-IB MTU and > they are presented with it? Let's look at one of the ports you did in > this patch as an example: > >> diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c >> index 21c551d..5a0656f 100644 >> --- a/examples/ud_pingpong.c >> +++ b/examples/ud_pingpong.c >> @@ -332,7 +332,7 @@ static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int size, >> fprintf(stderr, "Unable to query port info for port %d\n", port); >> goto clean_device; >> } >> - mtu = 1 << (port_info.active_mtu + 7); >> + mtu = ibv_mtu_to_num(port_info.active_mtu); >> if (size > mtu) { >> fprintf(stderr, "Requested size larger than port MTU (%d)\n", mtu); >> goto clean_device; > > That used to be a valid mathematical construct. Now it isn't. It will > work for all of the IBV_MTU_* values, but if you run this program, > unmodified, on an MTU 9000 link, you get 1 << 9007 ;-) > > Saying that this is backwards compatible is therefore incorrect. If it > were entirely backwards compatible, then apps would not need to be > recompiled in order to avoid this error. > > One possible solution to this problem is to use ld.linux's symbolic > symbol versions to solve this problem for us. Fortunately, Roland has > been excellent in the past about keeping all of libibverbs symbols > versioned. That can save us here. > > We would need to redefine the active_mtu and max_mtu in ibv_device_attr > and path_mtu in ibv_qp_attr to all be ints. No need to maintain back > compatibility. I should point out here that I would also change their name slightly in order to force current apps to fail to compile. This *is* an API change, and apps need to have to make the very minor touchups necessary in order to work again. You don't want someone to recompile their app without making this change and then be surprised. The other option is to add the int based MTUs as new elements and leave these, and also leave these elements as an enum, but in that case I would warn people that use these items in some way (a compiler warning about touching an item that's deprecated might be good if that's even possible to do). > Then we define ibv_device_attr_1.1 and ibv_qp_attr_1.1 structs that use > the old enum. > > Then we define version IBVERBS_1.2 version of ibv_get_device_list plus a > version 1.2 of any other symbols that pass around ibv_device_attr struct > and ibv_qp_attr struct. The new default will be to use the new structs > that have MTUs defined as ints, but the old 1.1 version of things will > use the compat structs to pass things around. When we query the kernel > about a device/qp, if we are linked against an old app we will be using > the compat struct and we can do the conversion from int MTU to ibv_enum > based MTU and vice versa in the IBVERBS_1.1 wrapper functions that need > to be called to convert ints to enums and vice versa. > > It's a lot more work, but it's the right way to do this. > > So, sorry Jeff, but I'm going to Nack this patch as it stands on design > and back compatibility. This really needs an API update, and it can be > done in a back compatible way by using the shared library symbol version > mapping and compat wrapper functions. > > -- > 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 > -- 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
On Thu, Jun 20, 2013 at 12:34:04PM -0400, Doug Ledford wrote: > On 06/20/2013 10:21 AM, Jeff Squyres wrote: > > Keep IBV_MTU_* enums values as they are, but pass MTU values around as > > int's. This is an ABI-compatible change; legacy applications will use > > the enum values, > > I'm not really concerned with what the legacy apps use so much as what > they are presented with. In other words, I'm sure they won't request > anything other than an MTU represented by one of the enum values. The > problem though, is what if they are run on a link with a non-IB MTU and > they are presented with it? Let's look at one of the ports you did in > this patch as an example: Remember, apps will only see a wonky value if they are being used on one of Jeff's new not-IB, not-ROCE, not-iWARP transports. Who knows if they will even work on this new transport unmodified anyhow?? An app update to suport future transports is not unreasonable, it happened for iwarp, rocee, etc. > > diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c > > index 21c551d..5a0656f 100644 > > +++ b/examples/ud_pingpong.c > > @@ -332,7 +332,7 @@ static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int size, > > fprintf(stderr, "Unable to query port info for port %d\n", port); > > goto clean_device; > > } > > - mtu = 1 << (port_info.active_mtu + 7); .. and this is sketchy anyhow, the above maths are not defined to work anywhere, it just happens to work with the constants that have been defined so far. This would break equally if we added any new constant to the enum. So no, these maths are not important. > One possible solution to this problem is to use ld.linux's symbolic > symbol versions to solve this problem for us. Fortunately, Roland has > been excellent in the past about keeping all of libibverbs symbols > versioned. That can save us here. There is a huge resistance to reving the symbol versions in ibverbs. See the whole extension mess. Further, the symbol versions don't work well in verbs, the internal structures are too exposed. The existing support is already broken and only works in very limited cases. What you propose breaks in fairly common use cases, eg if librdmacm/etc and the app link to different ibverbs versions then things go wrong. rdmacm and the app pass pointers to verbs structures across their boundary but they are unware they are versioned differently, and will pass them back to the wrong verbs entry point. This has already been seen to fail with the existing symbol version support. Basically: the verbs ABI was not designed to work with symbol versions. Jason -- 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
On 06/20/2013 12:53 PM, Jason Gunthorpe wrote: > On Thu, Jun 20, 2013 at 12:34:04PM -0400, Doug Ledford wrote: >> On 06/20/2013 10:21 AM, Jeff Squyres wrote: >>> Keep IBV_MTU_* enums values as they are, but pass MTU values around as >>> int's. This is an ABI-compatible change; legacy applications will use >>> the enum values, >> >> I'm not really concerned with what the legacy apps use so much as what >> they are presented with. In other words, I'm sure they won't request >> anything other than an MTU represented by one of the enum values. The >> problem though, is what if they are run on a link with a non-IB MTU and >> they are presented with it? Let's look at one of the ports you did in >> this patch as an example: > > Remember, apps will only see a wonky value if they are being used on > one of Jeff's new not-IB, not-ROCE, not-iWARP transports. So? That's just today. The only reason RoCE/IBoE maps to IB MTUs is that they didn't bother to make this ABI break for it, but it could benefit from having a more flexible MTU that followed the underlying Ethernet MTU. So who's to say that isn't next? > Who knows if > they will even work on this new transport unmodified anyhow?? Either we should be trying to keep back compatibility or we shouldn't. If we are, then it should work. If we aren't, then there is no sense doing the magic hocus-pocus tricks with the MTU where in some cases it is the old enum value and other cases the real MTU value. > An app update to suport future transports is not unreasonable, I disagree. > it > happened for iwarp, rocee, etc. If it happened once, then I would agree with you above. That it *keeps* happening is the issue. To me, that's a clear indication that instead of fixing the shortcomings of the current API properly, band-aids just keep getting applied. >>> diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c >>> index 21c551d..5a0656f 100644 >>> +++ b/examples/ud_pingpong.c >>> @@ -332,7 +332,7 @@ static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int size, >>> fprintf(stderr, "Unable to query port info for port %d\n", port); >>> goto clean_device; >>> } >>> - mtu = 1 << (port_info.active_mtu + 7); > > .. and this is sketchy anyhow, the above maths are not defined to work > anywhere, it just happens to work with the constants that have been > defined so far. This would break equally if we added any new constant > to the enum. So no, these maths are not important. No, but I also skipped a number of patches where code did switch statements to convert from enum to byte value, or enum to string representation. All of those would break too. >> One possible solution to this problem is to use ld.linux's symbolic >> symbol versions to solve this problem for us. Fortunately, Roland has >> been excellent in the past about keeping all of libibverbs symbols >> versioned. That can save us here. > > There is a huge resistance to reving the symbol versions in > ibverbs. See the whole extension mess. I thought the resistance was to revving the libibverbs soname, not just the internal symbol versions. > Further, the symbol versions don't work well in verbs, the internal > structures are too exposed. The existing support is already broken and > only works in very limited cases. > > What you propose breaks in fairly common use cases, eg if > librdmacm/etc and the app link to different ibverbs versions then > things go wrong. At the time the app is compiled, it will be compiled against a librdmacm that needs a specific version of the libibverbs symbols because librdmacm has already been compiled. That means that if you want things to "just work" for the end user, when you rev the internal libibverbs symbols, then you make a corresponding change in librdmacm and when you install libibverbs-devel, you make it have a Conflict: with librdmacm < new-version. Likewise, you make librdmacm have a BuildRequires: libibverbs-devel >= new-version, and make librdmacm-devel have a Requires: libibverbs-devel >= new-version. In this way, librdmacm-devel will automatically require that the installed libibverbs-devel be of the right version or it won't install itself. Likewise, updating libibverbs-devel without also updating librdmacm-devel will cause the entire transaction to get kicked out or, depending on options, cause librdmacm to be removed from the system to be updated later. Now, these are package install time checks that can be implemented in either rpm or, I assume, apt. If you want compile time checks, that could be done too with header file magic. So, this isn't broken, it's just that no one is taking the time to properly identify incompatible versions and force compatible versions to be installed before things are allowed to link up. > rdmacm and the app pass pointers to verbs structures > across their boundary but they are unware they are versioned > differently, and will pass them back to the wrong verbs entry > point. This has already been seen to fail with the existing symbol > version support. > > Basically: the verbs ABI was not designed to work with symbol > versions. The verbs ABI is perfectly fine working with versioned symbols. The package management of interrelated libraries has not been managed sufficiently to provide even a modicum of assurance that things will work properly. This isn't for lack of tools, it's most likely simply for lack of knowledge of how to, and desire to, deal with sorting the issues out. -- 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
On Thu, Jun 20, 2013 at 04:31:14PM -0400, Doug Ledford wrote: > > happened for iwarp, rocee, etc. > > If it happened once, then I would agree with you above. That it *keeps* > happening is the issue. To me, that's a clear indication that instead > of fixing the shortcomings of the current API properly, band-aids just > keep getting applied. The new transports have new requirements, and the apps have new required behaviors - the API simply can't hide all this in every case. The changes before had nothing to do with MTU, FWIW. Jeff: Does your new transport support 100% of ibverbs and MTU is the only change an app would need? > > .. and this is sketchy anyhow, the above maths are not defined to work > > anywhere, it just happens to work with the constants that have been > > defined so far. This would break equally if we added any new constant > > to the enum. So no, these maths are not important. > > No, but I also skipped a number of patches where code did switch > statements to convert from enum to byte value, or enum to string > representation. All of those would break too. Yes, but often either doesn't matter (they are just print strings) or there are default fall throughs. UD apps are ones that are going to have a problem, but we already have very poor transport agnostic support for UD, so it is unlikely an existing UD app will run on a new transport. > > There is a huge resistance to reving the symbol versions in > > ibverbs. See the whole extension mess. > > I thought the resistance was to revving the libibverbs soname, not just > the internal symbol versions. Nope, people want new apps (using extensions/etc) to run on old verbs versions. I don't really like that, mind you, but it has been strongly asked for. > At the time the app is compiled, it will be compiled against a librdmacm > that needs a specific version of the libibverbs symbols because > librdmacm has already been compiled. That means that if you want > things to "just work" for the end user, when you rev the internal libibverbs > symbols, then you make a corresponding change in librdmacm and when > you Both the app and librdmacm have a DT_NEEDED on libibverbs, and both call into libibverbs. The issue is not sorting out the install of the core libraries via package management tricks, but what happens when an app/middleware outside the package management dynamically links to this mess. We've already seen this fail in the field with apps that link to the v1.0 verbs ABI that call into other libraries that were linked to the v1.1 API. It explodes. The fundamental problem with the v1.0/v1.1 switch is the v1.0 functions are returning pointers that cannot be passed into a v1.1 function, eg iv_close_device@1.1(ibv_open_device@1.0(..)) crashes. Your idea to change the MTU causes the same problem with structure versioning. If I use a rdmacm/etc API to get a MTU containing structure then I still get the new meaning because rdmacm is linked to the v1.2 verbs symbols, but my app is linked to the v1.1 symbols and can't support it. .. and of course rdmacm is just an example, there are other middleware libraries (uDAPL, MPI, etc) that may be affected. Symbol versioning *doesn't* solve the problem, it just creates a new class of subtle failure modes. It appears to work in simple cases so people think it is a silver bullet, but it is not. It is very complex, the failures cases are screwy and subtle, and verbs tends to hit them head on because of how exposed all the internal structures are. > So, this isn't broken, it's just that no one is taking the time to > properly identify incompatible versions and force compatible versions to > be installed before things are allowed to link up. You can't enforce things on binary-only proprietary apps being installed from outside package management. The verbs extension mechanism can safely deal with this kind of change, it effectively adds structure versioning to the ABI, but it is not mainlined yet and is also pretty complex. Jason -- 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
On 06/20/2013 05:14 PM, Jason Gunthorpe wrote: > On Thu, Jun 20, 2013 at 04:31:14PM -0400, Doug Ledford wrote: >>> happened for iwarp, rocee, etc. >> >> If it happened once, then I would agree with you above. That it *keeps* >> happening is the issue. To me, that's a clear indication that instead >> of fixing the shortcomings of the current API properly, band-aids just >> keep getting applied. > > The new transports have new requirements, and the apps have new > required behaviors - the API simply can't hide all this in every > case. The changes before had nothing to do with MTU, FWIW. It demonstrates what I would call a leakage between layer 2 and higher layer APIs though. > Nope, people want new apps (using extensions/etc) to run on old verbs > versions. I don't really like that, mind you, but it has been strongly > asked for. At some point people just need to suck it up and deal with a new version. Once you reach a certain level of maturity you can maintain long term back compatibility and forward compatibility. But that requires that the API be sufficiently well thought out that each change is more evolutionary than revolutionary. The entire IB stack still likes to do major, revolutionary changes. It has not reached the level of maturity where it can truly maintain a long term forward/back compatibility IMO. And the layer level leakage I mention earlier just makes this more problematic. > Both the app and librdmacm have a DT_NEEDED on libibverbs, and both > call into libibverbs. > > The issue is not sorting out the install of the core libraries via > package management tricks, but what happens when an app/middleware > outside the package management dynamically links to this mess. If a user chooses not to use packaging, that's their prerogative. However, they can also collect the pieces when things break. If a ISV chooses to do the same, then that ISV is just being flat lazy and sloppy. The package management stacks are there for a reason and serve a valuable purpose. Ignoring them is akin to just thumbing your nose at the libibverbs version as well. > We've already seen this fail in the field with apps that link to the > v1.0 verbs ABI that call into other libraries that were linked to the > v1.1 API. So this exposes an issue, I agree. > It explodes. The fundamental problem with the v1.0/v1.1 switch is the > v1.0 functions are returning pointers that cannot be passed into a > v1.1 function, eg iv_close_device@1.1(ibv_open_device@1.0(..)) > crashes. This isn't a problem if library A doesn't call into library B and try to use the same struct as the app itself when the app calls into library B. > Your idea to change the MTU causes the same problem with structure > versioning. If I use a rdmacm/etc API to get a MTU containing > structure then I still get the new meaning because rdmacm is linked to > the v1.2 verbs symbols, but my app is linked to the v1.1 symbols and > can't support it. > > .. and of course rdmacm is just an example, there are other middleware > libraries (uDAPL, MPI, etc) that may be affected. > > Symbol versioning *doesn't* solve the problem, it just creates a new > class of subtle failure modes. It appears to work in simple cases so > people think it is a silver bullet, but it is not. It is very complex, > the failures cases are screwy and subtle, and verbs tends to hit them > head on because of how exposed all the internal structures are. I would argue that this is because the libraries are so disjoint (that librdmacm needs the deep internal knowledge it needs of libibverbs indicates that maybe these two shouldn't be separate from each other for example, or that maybe libibverbs should provide a unified connection API to the user and internally use both librdmacm and libibcm on the back end to work IP v. GUID connection setup). So, I think there is significant room to improve the layout of the overall RDMA APIs and doing that would address this particular issue and is probably the right way to go. However, aside from that, my current objection to all of this is that this solution, while meeting the needs of the "we don't want to have to change anything unless the app wants to run on this new fabric" results in what I would call a gross hack (some enum, some int, same variable). I'm not so much complaining about Jeff's solution, more the requirement that we come up with such an ugly construct. We are headed down a course of putting in gross hacks in order to preserve an outdated design, one which has much more elegant solutions today than what we are currently using. At *some* point, this becomes a miserable, unmaintainable mess. So I hear you that people object to breaking the API for a new library version. My objection (which I'm sure I'll be overruled on) is that people are taking the easy way out instead of fixing things up the right way. >> So, this isn't broken, it's just that no one is taking the time to >> properly identify incompatible versions and force compatible versions to >> be installed before things are allowed to link up. > > You can't enforce things on binary-only proprietary apps being > installed from outside package management. Correcting the API resolves this, and you can possibly play games in header files to catch issues at compile time. But if people ignore package management, then they get to keep their own pieces as far as I'm concerned. > The verbs extension mechanism can safely deal with this kind of > change, it effectively adds structure versioning to the ABI, but it is > not mainlined yet and is also pretty complex. That would address structures, but I think the API itself could use some love and care, and that wouldn't be addressed by just the verbs extension mechanism (and in fact if you rethink some of the exposed API, it might drastically change how you might want to handle extensions...who knows). -- 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
> I would argue that this is because the libraries are so disjoint (that > librdmacm needs the deep internal knowledge it needs of libibverbs > indicates that maybe these two shouldn't be separate from each other for > example, or that maybe libibverbs should provide a unified connection > API to the user and internally use both librdmacm and libibcm on the > back end to work IP v. GUID connection setup). So, I think there is > significant room to improve the layout of the overall RDMA APIs and > doing that would address this particular issue and is probably the right > way to go. ... > That would address structures, but I think the API itself could use some > love and care, and that wouldn't be addressed by just the verbs > extension mechanism (and in fact if you rethink some of the exposed API, > it might drastically change how you might want to handle > extensions...who knows). I agree with Doug. A merged library that can evolve the RDMA APIs with fewer compatibility constraints could be beneficial. I just think such an approach would require some thought and a lot of discussion. - Sean -- 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
On Thu, Jun 20, 2013 at 08:31:07PM -0400, Doug Ledford wrote: > > The new transports have new requirements, and the apps have new > > required behaviors - the API simply can't hide all this in every > > case. The changes before had nothing to do with MTU, FWIW. > > It demonstrates what I would call a leakage between layer 2 and higher > layer APIs though. What do you mean? Verbs is intended to expose transport specific behaviors at a very bare-metal level. The fact there are wide variations between the transports doesn't reflect a fault in verbs. If you want a transport-agnostic API then there are projects that do that: sockets, MPI, portals, CCI, etc, etc. But look at some of the new extensions people are proposing, they tend to be very transport specific, narrowly focused on certain performance niches and not really abstractable through general APIs. Frankly, that is why they are getting shoved into verbs, not run over sockets :) > > The issue is not sorting out the install of the core libraries via > > package management tricks, but what happens when an app/middleware > > outside the package management dynamically links to this mess. > > If a user chooses not to use packaging, that's their prerogative. > However, they can also collect the pieces when things break. If a ISV > chooses to do the same, then that ISV is just being flat lazy and > sloppy. The package management stacks are there for a reason and serve > a valuable purpose. Ignoring them is akin to just thumbing your nose at > the libibverbs version as well. The packaging tool still doesn't solve the problem I outlined. A correctly packaged app, built for verbs v1.0 will still be installable on a system with verbs v1.1, and the same inter-library problems I described with symbol versioning in verbs will still show up. You can avoid some nasty cases in the core libraries themselves with packaging, but it doesn't solve the general problem. .. and ISVs don't seem to like packaging for some insane reason. > > It explodes. The fundamental problem with the v1.0/v1.1 switch is the > > v1.0 functions are returning pointers that cannot be passed into a > > v1.1 function, eg iv_close_device@1.1(ibv_open_device@1.0(..)) > > crashes. > > This isn't a problem if library A doesn't call into library B and try to > use the same struct as the app itself when the app calls into library B. K, but they do, there are good reasons why they do, and saying "don't do that" is really not helpful. > I would argue that this is because the libraries are so disjoint (that > librdmacm needs the deep internal knowledge it needs of libibverbs No, it isn't deep internal knowledge. It uses the exposed, defined, public verbs ABI, adds some helper functions and then re-exports verbs objects to it's own users with minimal overhead. The re-exporting is what burns symbol versions so badly. And rdma cm isn't the only one, but it is the easiest to talk about. IMHO, the #1 fundamental issue is that all the low speed APIs use exposed structures, often caller-stack-allocated and that very badly limits our ability to elegantly adjust the API without breaking the ABI. > back end to work IP v. GUID connection setup). So, I think there is > significant room to improve the layout of the overall RDMA APIs and > doing that would address this particular issue and is probably the right > way to go. I won't argue with you there... > However, aside from that, my current objection to all of this is that > this solution, while meeting the needs of the "we don't want to have to > change anything unless the app wants to run on this new fabric" results > in what I would call a gross hack (some enum, some int, same variable). > I'm not so much complaining about Jeff's solution, more the requirement > that we come up with such an ugly construct. We are headed down a > course of putting in gross hacks in order to preserve an outdated > design, one which has much more elegant solutions today than what we are > currently using. At *some* point, this becomes a miserable, > unmaintainable mess. At some point? We are already there! Have you looked at the extension mechanism? It is horrible, and not what any sane person would want to do. It exists soley to satisfy the ISVs that don't want to see verbs rev'd. And they have a point. There are lot of things built on verbs, a rev to the soname would create a terrible mess. An incompatible rev to the API would be even worse. > So I hear you that people object to breaking the API for a new library > version. My objection (which I'm sure I'll be overruled on) is that > people are taking the easy way out instead of fixing things up the right > way. So what is the 'right way' here? I'm not hearing any problem free solution. Jason -- 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
On 06/21/2013 02:36 AM, Jason Gunthorpe wrote: > On Thu, Jun 20, 2013 at 08:31:07PM -0400, Doug Ledford wrote: > >>> The new transports have new requirements, and the apps have new >>> required behaviors - the API simply can't hide all this in every >>> case. The changes before had nothing to do with MTU, FWIW. >> >> It demonstrates what I would call a leakage between layer 2 and higher >> layer APIs though. > > What do you mean? Verbs is intended to expose transport specific > behaviors at a very bare-metal level. The fact there are wide > variations between the transports doesn't reflect a fault in verbs. When verbs only had to worry about IB, this made sense. When we added iWARP, IBoE, and now usNIC, it no longer does. Verbs *needs* to be transport agnostic. Then a person should do non-agnostic things using extension libraries or raw packet mode, similar to how transport non-agnostic things are done for the sockets API. > If you want a transport-agnostic API then there are projects that do > that: sockets, MPI, portals, CCI, etc, etc. > > But look at some of the new extensions people are proposing, they tend > to be very transport specific, narrowly focused on certain performance > niches and not really abstractable through general APIs. > > Frankly, that is why they are getting shoved into verbs, not run over > sockets :) I wouldn't care about verbs being transport agnostic if we already had a reasonable transport agnostic API for RDMA usage that allowed all of the base verbs to be used. I don't see that from the examples you list above. >>> The issue is not sorting out the install of the core libraries via >>> package management tricks, but what happens when an app/middleware >>> outside the package management dynamically links to this mess. >> >> If a user chooses not to use packaging, that's their prerogative. >> However, they can also collect the pieces when things break. If a ISV >> chooses to do the same, then that ISV is just being flat lazy and >> sloppy. The package management stacks are there for a reason and serve >> a valuable purpose. Ignoring them is akin to just thumbing your nose at >> the libibverbs version as well. > > The packaging tool still doesn't solve the problem I outlined. A > correctly packaged app, built for verbs v1.0 will still be installable > on a system with verbs v1.1, and the same inter-library problems I > described with symbol versioning in verbs will still show up. Not if you also version the other libraries (and I admit you are getting into a lot of work here, but what I'm referring to is when you rev ibv_get_device_list for the new ibv_device_attr struct, you also rev rdma_get_device_list in the same way, keeping a back compat entry in rdmacm as well). > You can avoid some nasty cases in the core libraries themselves with > packaging, but it doesn't solve the general problem. > > .. and ISVs don't seem to like packaging for some insane reason. > >>> It explodes. The fundamental problem with the v1.0/v1.1 switch is the >>> v1.0 functions are returning pointers that cannot be passed into a >>> v1.1 function, eg iv_close_device@1.1(ibv_open_device@1.0(..)) >>> crashes. >> >> This isn't a problem if library A doesn't call into library B and try to >> use the same struct as the app itself when the app calls into library B. > > K, but they do, there are good reasons why they do, and saying "don't > do that" is really not helpful. Except in the context of, as Sean picked up on, my suggestion that reworking the API split a bit and bringing these highly related items under one umbrella. You wouldn't expect to link against glibc for read/write/socket, and against a different library for listen/accept, yet that's what we do in rdma land. We have an artificial split that doesn't make sense and it causes these problems. >> I would argue that this is because the libraries are so disjoint (that >> librdmacm needs the deep internal knowledge it needs of libibverbs > > No, it isn't deep internal knowledge. It uses the exposed, defined, > public verbs ABI, adds some helper functions and then re-exports verbs > objects to it's own users with minimal overhead. The re-exporting is > what burns symbol versions so badly. > > And rdma cm isn't the only one, but it is the easiest to talk > about. > > IMHO, the #1 fundamental issue is that all the low speed APIs use > exposed structures, often caller-stack-allocated and that very badly > limits our ability to elegantly adjust the API without breaking the ABI. > >> back end to work IP v. GUID connection setup). So, I think there is >> significant room to improve the layout of the overall RDMA APIs and >> doing that would address this particular issue and is probably the right >> way to go. > > I won't argue with you there... > >> However, aside from that, my current objection to all of this is that >> this solution, while meeting the needs of the "we don't want to have to >> change anything unless the app wants to run on this new fabric" results >> in what I would call a gross hack (some enum, some int, same variable). >> I'm not so much complaining about Jeff's solution, more the requirement >> that we come up with such an ugly construct. We are headed down a >> course of putting in gross hacks in order to preserve an outdated >> design, one which has much more elegant solutions today than what we are >> currently using. At *some* point, this becomes a miserable, >> unmaintainable mess. > > At some point? We are already there! Have you looked at the extension > mechanism? It is horrible, and not what any sane person > would want to do. It exists soley to satisfy the ISVs that don't want > to see verbs rev'd. > > And they have a point. There are lot of things built on verbs, a rev > to the soname would create a terrible mess. An incompatible rev to the > API would be even worse. I disagree. For end users, that *vast* majority would never have to see the API change. The MPI stacks and a few ISV stacks would need updated, they would change their internal implementations but not necessarily anything externally visible (for instance, OpenMPI would not need to change the MPI interface to update to a newer verbs provider, and MPI programs linked against OpenMPI would not need to be changed at all), and I'm guessing that 99% of all applications by CPU hours consumed would end up magically updated to the latest version. Or am I wrong that by far and away the two largest uses of RDMA in general are A) MPI stacks and B) messaging stacks in financial sectors...both of which I know for a fact hide the actual RDMA API from the end user's code? >> So I hear you that people object to breaking the API for a new library >> version. My objection (which I'm sure I'll be overruled on) is that >> people are taking the easy way out instead of fixing things up the right >> way. > > So what is the 'right way' here? I'm not hearing any problem free > solution. Who ever said that the right solution is either A) problem free or B) always easy? If you're looking for a for-free solution, don't bother asking me. As I outlined above, it seems to me that you can catch 99% of the target audience with a limited set of targeted provider stack updates. This is totally doable, although not necessarily easy. The remaining code will have to be done by the end users. If people think that's too much work to fix the mess things are in currently, my response would be "Suck it up, cupcake! It needs done." You could even leave the current libibverbs/librdmacm/libibcm combo in place and allow non-updated programs to continue working, while providing a new librdma that handles all three jobs, but in a transport agnostic way. Then you could add transport specific extensions to librdma to get those extra features you want. That way, if people who haven't updated want new features, then they can take the time to do the forward port to the new library, and if not they can continue to use the deprecated but still present older libraries. So I guess I'm failing to really see the issue that people keep making this out to be... -- 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
On Fri, Jun 21, 2013 at 01:36:01PM -0400, Doug Ledford wrote: > >>> The new transports have new requirements, and the apps have new > >>> required behaviors - the API simply can't hide all this in every > >>> case. The changes before had nothing to do with MTU, FWIW. > >> > >> It demonstrates what I would call a leakage between layer 2 and higher > >> layer APIs though. > > > > What do you mean? Verbs is intended to expose transport specific > > behaviors at a very bare-metal level. The fact there are wide > > variations between the transports doesn't reflect a fault in verbs. > > When verbs only had to worry about IB, this made sense. When we added > iWARP, IBoE, and now usNIC, it no longer does. Verbs *needs* to be > transport agnostic. Then a person should do non-agnostic things using > extension libraries or raw packet mode, similar to how transport > non-agnostic things are done for the sockets API. I disagree, *verbs* needs to expose the HW capabilities, efficiently. .. and what is this about 'extension libraries'!? rdmacm *IS* an extension library and here we are talking about the problems that causes. You want more extension libraries? .. and raw packet mode is entirely a non-starter for controller HW assisted offload features. Like I said before, these things are getting pushed into verbs because they don't fit in the sockets model. You really can't use sockets as a template for solving problems that sockets have already failed to solve :P > I wouldn't care about verbs being transport agnostic if we already > had a reasonable transport agnostic API for RDMA usage that allowed > all of the base verbs to be used. I don't see that from the > examples you list above. There is some stuff in the latest rdma-cm library that is getting close to this, but you are broadly right. It is a hard problem, especially when new transports don't support many of the verbs. > > The packaging tool still doesn't solve the problem I outlined. A > > correctly packaged app, built for verbs v1.0 will still be installable > > on a system with verbs v1.1, and the same inter-library problems I > > described with symbol versioning in verbs will still show up. > > Not if you also version the other libraries (and I admit you are getting > into a lot of work here, but what I'm referring to is when you rev > ibv_get_device_list for the new ibv_device_attr struct, you also rev > rdma_get_device_list in the same way, keeping a back compat entry in > rdmacm as well). Yes, that is right, plus actually making compat calls across the two library boundaries and other complexity. It is certainly a big job. > >> This isn't a problem if library A doesn't call into library B and try to > >> use the same struct as the app itself when the app calls into library B. > > > > K, but they do, there are good reasons why they do, and saying "don't > > do that" is really not helpful. > > Except in the context of, as Sean picked up on, my suggestion that > reworking the API split a bit and bringing these highly related items > under one umbrella. I agree that combining the core libraries would simplify the maintenance, but it you are still talking about adding symbol versions to a big wack of APIs, and that still leaves non-core helper libraries out in the cold. Again, the problems are not caused by the split up libraries. The problems are inherent to how the verbs API works. Joining the libraries reduces the 'impact surface' but doesn't fundamentally solve the problem. Perhaps that is pragmatically '99%' good enough, as you say.. Admittedly, this seems to be how other projects use symbol versions, and I don't hear alot of screaming from other camps, but on the other hand, verbs has a larger than normal binary-only ISV user base.. .. and symbol versions do work well if an entirely contained ISV build tries to link to a set of self-consistent system libraries. > You wouldn't expect to link against glibc for read/write/socket, and > against a different library for listen/accept, yet that's what we do > in rdma land. We have an artificial split that doesn't make sense > and it causes these problems. Have you used SCTP? Some calls are part of glibc, some are part of libsctp.. For instance listen is in glibc, and sctp_send is in libsctp. It isn't entirely unprecedented. > >> So I hear you that people object to breaking the API for a new library > >> version. My objection (which I'm sure I'll be overruled on) is that > >> people are taking the easy way out instead of fixing things up the right > >> way. > > > > So what is the 'right way' here? I'm not hearing any problem free > > solution. > > Who ever said that the right solution is either A) problem free or B) > always easy? If you're looking for a for-free solution, don't bother > asking me. As I outlined above, it seems to me that you can catch 99% > of the target audience with a limited set of targeted provider stack > updates. This is totally doable, although not necessarily easy. The > remaining code will have to be done by the end users. If people think > that's too much work to fix the mess things are in currently, my > response would be "Suck it up, cupcake! It needs done." You could > even K.. So, nobody seems to be paid to work on these verbs libraries, what you are talking about is a tonne of typing - who is going to do this? I already tried pretty hard to convince some ISVs that it was OK to rev the SONAME and the extensions could be handled with new symbols, and I didn't get too far. :( In the mean time, what about Jeff's problem? Jason -- 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
On 06/21/2013 02:26 PM, Jason Gunthorpe wrote: > On Fri, Jun 21, 2013 at 01:36:01PM -0400, Doug Ledford wrote: > >>>>> The new transports have new requirements, and the apps have new >>>>> required behaviors - the API simply can't hide all this in every >>>>> case. The changes before had nothing to do with MTU, FWIW. >>>> >>>> It demonstrates what I would call a leakage between layer 2 and higher >>>> layer APIs though. >>> >>> What do you mean? Verbs is intended to expose transport specific >>> behaviors at a very bare-metal level. The fact there are wide >>> variations between the transports doesn't reflect a fault in verbs. >> >> When verbs only had to worry about IB, this made sense. When we added >> iWARP, IBoE, and now usNIC, it no longer does. Verbs *needs* to be >> transport agnostic. Then a person should do non-agnostic things using >> extension libraries or raw packet mode, similar to how transport >> non-agnostic things are done for the sockets API. > > I disagree, *verbs* needs to expose the HW capabilities, efficiently. What we need is an efficient RDMA API (of which verbs is a candidate for the underlying implementation). It need not expose HW capabilities in its base form. > .. and what is this about 'extension libraries'!? rdmacm *IS* an > extension library and here we are talking about the problems that > causes. It should not be. There is no such thing as RDMA communications without connection establishment. It isn't really an extension, it's part of the core. > You want more extension libraries? Architecturally different than the libibverbs/librdmacm mess, but yes (think extensions that the core library offers to the user space app, not that the user space app links against and must be kept in sync with the core library). > .. and raw packet mode is entirely a non-starter for controller HW > assisted offload features. > > Like I said before, these things are getting pushed into verbs because > they don't fit in the sockets model. You really can't use sockets as a > template for solving problems that sockets have already failed to solve > :P I'm not using sockets as a model. I couldn't care less about them. I want a generic RDMA model and sockets does not work well for that (not at all really, unless you just want to neuter RDMA capabilities). >> I wouldn't care about verbs being transport agnostic if we already >> had a reasonable transport agnostic API for RDMA usage that allowed >> all of the base verbs to be used. I don't see that from the >> examples you list above. > > There is some stuff in the latest rdma-cm library that is getting > close to this, but you are broadly right. It is a hard problem, > especially when new transports don't support many of the verbs. However, given a minimal set of functioning verbs, some of the other verbs can be emulated. >> Except in the context of, as Sean picked up on, my suggestion that >> reworking the API split a bit and bringing these highly related items >> under one umbrella. > > I agree that combining the core libraries would simplify the > maintenance, but it you are still talking about adding symbol versions > to a big wack of APIs, and that still leaves non-core helper libraries > out in the cold. Depends on how you layer things as I mentioned above. > Again, the problems are not caused by the split up libraries. No, it partially is exactly caused by this. > The > problems are inherent to how the verbs API works. Nothing is inherent to the API, only it's current implementation. > Joining the > libraries reduces the 'impact surface' but doesn't fundamentally solve > the problem. Perhaps that is pragmatically '99%' good enough, as you > say.. > > Admittedly, this seems to be how other projects use symbol versions, > and I don't hear alot of screaming from other camps, but on the other > hand, verbs has a larger than normal binary-only ISV user base.. > > .. and symbol versions do work well if an entirely contained ISV build > tries to link to a set of self-consistent system libraries. > >> You wouldn't expect to link against glibc for read/write/socket, and >> against a different library for listen/accept, yet that's what we do >> in rdma land. We have an artificial split that doesn't make sense >> and it causes these problems. > > Have you used SCTP? Some calls are part of glibc, some are part of > libsctp.. For instance listen is in glibc, and sctp_send is in > libsctp. It isn't entirely unprecedented. Not-unprecedented still doesn't mean "good idea" ;-) >>>> So I hear you that people object to breaking the API for a new library >>>> version. My objection (which I'm sure I'll be overruled on) is that >>>> people are taking the easy way out instead of fixing things up the right >>>> way. >>> >>> So what is the 'right way' here? I'm not hearing any problem free >>> solution. >> >> Who ever said that the right solution is either A) problem free or B) >> always easy? If you're looking for a for-free solution, don't bother >> asking me. As I outlined above, it seems to me that you can catch 99% >> of the target audience with a limited set of targeted provider stack >> updates. This is totally doable, although not necessarily easy. The >> remaining code will have to be done by the end users. If people think >> that's too much work to fix the mess things are in currently, my >> response would be "Suck it up, cupcake! It needs done." You could >> even > > K.. So, nobody seems to be paid to work on these verbs libraries, what > you are talking about is a tonne of typing - who is going to do > this? <raises hand> I'm ready for a new project. I already had one in mind, but it required some changes to libibverbs that are pretty deep changes. Instead of doing that deep work in libibverbs, I can start here and move on to my next project when this is up and running. Realistically I'm talking about starting off by cannibalizing libibverbs, librdmacm, and libibcm into a single library. That's a lot of cut and paste work, with ensuing fixups. After the initial bring together, then we can start re-architecting and moving forward. Since that cut and paste work will take a while, that also gives some time for us to discuss what things might look like before starting that "tonne of typing" as you called it ;-) > I already tried pretty hard to convince some ISVs that it was OK to > rev the SONAME and the extensions could be handled with new symbols, > and I didn't get too far. :( If they wish to keep getting their infrastructure for free, then they kinda get what they get. I mean, you don't want to just change things and piss them off for no good reason, but when good reason exists, they can suck it up and deal with it. No one in the open source community is obligated to preserve an outdated API forever just to make them happy. The other alternative is that they can pick up long term maintenance of libibverbs, librdmacm, and libibcm themselves (in fairness I can't say that...Roland may think it is a horrible idea to put connection management into the base verbs library and just want to keep on going the way things are going, ditto to Sean, I obviously can't speak for other people...but if we write a new library and the other maintainers of the older libraries think the new one is the right path forward and decide to quite updating the old ones, then you end up in this situation). They wouldn't be left with no choice, just with no choice that includes someone providing them with ongoing feature enhancements to an increasingly inappropriate and unmaintainable mess of an API for free. > In the mean time, what about Jeff's problem? We did get way off the point ;-) As I mentioned, this patch is just another in a line of kludges. That doesn't make it wrong, it makes it ugly. I'm for breaking the cycle. But even if we do go ahead and break the cycle, that will take more time than I imagine Jeff is willing to wait. So in the meantime, I guess we just gotta do what we just gotta do... -- 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
On Fri, Jun 21, 2013 at 04:57:09PM -0400, Doug Ledford wrote: > > I disagree, *verbs* needs to expose the HW capabilities, efficiently. > > What we need is an efficient RDMA API (of which verbs is a candidate for > the underlying implementation). It need not expose HW capabilities in > its base form. Well, there is uDAPL, but lots of people don't like it... > > K.. So, nobody seems to be paid to work on these verbs libraries, what > > you are talking about is a tonne of typing - who is going to do > > this? > > <raises hand> I'm ready for a new project. I already had one in mind, > but it required some changes to libibverbs that are pretty deep changes. > Instead of doing that deep work in libibverbs, I can start here and > move on to my next project when this is up and running. That would be amazing, I will try to help you as best my limited time permits. > Realistically I'm talking about starting off by cannibalizing > libibverbs, librdmacm, and libibcm into a single library. That's a lot > of cut and paste work, with ensuing fixups. After the initial bring > together, then we can start re-architecting and moving forward. The core umad and the issm buisness should be included as well, eg single core library API that wraps the entire API the kernel exports from drivers/infiniband. I've done some elements of this in my python-rdma project. > If they wish to keep getting their infrastructure for free, then they > kinda get what they get. I mean, you don't want to just change things > and piss them off for no good reason, but when good reason exists, they > can suck it up and deal with it. No one in the open source community is > obligated to preserve an outdated API forever just to make them happy. > The other alternative is that they can pick up long term maintenance of > libibverbs, librdmacm, and libibcm themselves (in fairness I can't > say A fact of things right now is that the ISVs are contributing a majority of the manpower for changes to the API. > As I mentioned, this patch is just another in a line of kludges. That > doesn't make it wrong, it makes it ugly. I'm for breaking the cycle. > But even if we do go ahead and break the cycle, that will take more time > than I imagine Jeff is willing to wait. So in the meantime, I guess we > just gotta do what we just gotta do... K.. Jeff: If you are still reading - one concrete suggestion, I think, is to ensure compile-time failure when the new-format MTU variable is touched. This is trivially done by wrapping it in a struct: struct ibv_mtu_t {int __mtu;}; At least this will cause compile failure at all sites that need revision. Jason -- 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
> > <raises hand> I'm ready for a new project. I already had one in mind, > > but it required some changes to libibverbs that are pretty deep changes. > > Instead of doing that deep work in libibverbs, I can start here and > > move on to my next project when this is up and running. > > That would be amazing, I will try to help you as best my limited time > permits. I've given this some thought recently as well. The concepts that I envision would be fairly significant relative to the current API, but build off the libibverbs framework and encapsulate all of the kernel ABI as well. The obvious goal is to avoid introducing any performance impact. Working down to something concrete would definitely take some time. - Sean -- 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
On Jun 21, 2013, at 5:20 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > Jeff: If you are still reading - I am still reading, just didn't have much to contribute until now. :-) > one concrete suggestion, I think, is > to ensure compile-time failure when the new-format MTU variable is > touched. This is trivially done by wrapping it in a struct: > > struct ibv_mtu_t {int __mtu;}; Sure, I can work up a patch that does this. Do others agree? Roland?
diff --git a/Makefile.am b/Makefile.am index 40e83be..1159e55 100644 --- a/Makefile.am +++ b/Makefile.am @@ -54,7 +54,8 @@ man_MANS = man/ibv_asyncwatch.1 man/ibv_devices.1 man/ibv_devinfo.1 \ man/ibv_post_srq_recv.3 man/ibv_query_device.3 man/ibv_query_gid.3 \ man/ibv_query_pkey.3 man/ibv_query_port.3 man/ibv_query_qp.3 \ man/ibv_query_srq.3 man/ibv_rate_to_mult.3 man/ibv_reg_mr.3 \ - man/ibv_req_notify_cq.3 man/ibv_resize_cq.3 man/ibv_rate_to_mbps.3 + man/ibv_req_notify_cq.3 man/ibv_resize_cq.3 man/ibv_rate_to_mbps.3 \ + man/ibv_mtu_to_num.3 DEBIAN = debian/changelog debian/compat debian/control debian/copyright \ debian/ibverbs-utils.install debian/libibverbs1.install \ diff --git a/examples/devinfo.c b/examples/devinfo.c index ff078e4..9f51dcb 100644 --- a/examples/devinfo.c +++ b/examples/devinfo.c @@ -111,18 +111,6 @@ static const char *atomic_cap_str(enum ibv_atomic_cap atom_cap) } } -static const char *mtu_str(enum ibv_mtu max_mtu) -{ - switch (max_mtu) { - case IBV_MTU_256: return "256"; - case IBV_MTU_512: return "512"; - case IBV_MTU_1024: return "1024"; - case IBV_MTU_2048: return "2048"; - case IBV_MTU_4096: return "4096"; - default: return "invalid MTU"; - } -} - static const char *width_str(uint8_t width) { switch (width) { @@ -301,10 +289,10 @@ static int print_hca_cap(struct ibv_device *ib_dev, uint8_t ib_port) printf("\t\tport:\t%d\n", port); printf("\t\t\tstate:\t\t\t%s (%d)\n", port_state_str(port_attr.state), port_attr.state); - printf("\t\t\tmax_mtu:\t\t%s (%d)\n", - mtu_str(port_attr.max_mtu), port_attr.max_mtu); - printf("\t\t\tactive_mtu:\t\t%s (%d)\n", - mtu_str(port_attr.active_mtu), port_attr.active_mtu); + printf("\t\t\tmax_mtu:\t\t%d (%d)\n", + ibv_mtu_to_num(port_attr.max_mtu), port_attr.max_mtu); + printf("\t\t\tactive_mtu:\t\t%d (%d)\n", + ibv_mtu_to_num(port_attr.active_mtu), port_attr.active_mtu); printf("\t\t\tsm_lid:\t\t\t%d\n", port_attr.sm_lid); printf("\t\t\tport_lid:\t\t%d\n", port_attr.lid); printf("\t\t\tport_lmc:\t\t0x%02x\n", port_attr.lmc); diff --git a/examples/pingpong.c b/examples/pingpong.c index 90732ef..d1c22c9 100644 --- a/examples/pingpong.c +++ b/examples/pingpong.c @@ -36,18 +36,6 @@ #include <stdio.h> #include <string.h> -enum ibv_mtu pp_mtu_to_enum(int mtu) -{ - switch (mtu) { - case 256: return IBV_MTU_256; - case 512: return IBV_MTU_512; - case 1024: return IBV_MTU_1024; - case 2048: return IBV_MTU_2048; - case 4096: return IBV_MTU_4096; - default: return -1; - } -} - uint16_t pp_get_local_lid(struct ibv_context *context, int port) { struct ibv_port_attr attr; diff --git a/examples/pingpong.h b/examples/pingpong.h index 9cdc03e..91d217b 100644 --- a/examples/pingpong.h +++ b/examples/pingpong.h @@ -35,7 +35,6 @@ #include <infiniband/verbs.h> -enum ibv_mtu pp_mtu_to_enum(int mtu); uint16_t pp_get_local_lid(struct ibv_context *context, int port); int pp_get_port_info(struct ibv_context *context, int port, struct ibv_port_attr *attr); diff --git a/examples/rc_pingpong.c b/examples/rc_pingpong.c index 15494a1..2d6d30e 100644 --- a/examples/rc_pingpong.c +++ b/examples/rc_pingpong.c @@ -78,7 +78,7 @@ struct pingpong_dest { }; static int pp_connect_ctx(struct pingpong_context *ctx, int port, int my_psn, - enum ibv_mtu mtu, int sl, + ibv_mtu_t mtu, int sl, struct pingpong_dest *dest, int sgid_idx) { struct ibv_qp_attr attr = { @@ -209,7 +209,7 @@ out: } static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx, - int ib_port, enum ibv_mtu mtu, + int ib_port, ibv_mtu_t mtu, int port, int sl, const struct pingpong_dest *my_dest, int sgid_idx) @@ -547,7 +547,7 @@ int main(int argc, char *argv[]) int port = 18515; int ib_port = 1; int size = 4096; - enum ibv_mtu mtu = IBV_MTU_1024; + ibv_mtu_t mtu = num_to_ibv_mtu(1024); int rx_depth = 500; int iters = 1000; int use_event = 0; @@ -608,7 +608,7 @@ int main(int argc, char *argv[]) break; case 'm': - mtu = pp_mtu_to_enum(strtol(optarg, NULL, 0)); + mtu = num_to_ibv_mtu(strtol(optarg, NULL, 0)); if (mtu < 0) { usage(argv[0]); return 1; diff --git a/examples/srq_pingpong.c b/examples/srq_pingpong.c index 6e00f8c..44915a7 100644 --- a/examples/srq_pingpong.c +++ b/examples/srq_pingpong.c @@ -81,7 +81,7 @@ struct pingpong_dest { union ibv_gid gid; }; -static int pp_connect_ctx(struct pingpong_context *ctx, int port, enum ibv_mtu mtu, +static int pp_connect_ctx(struct pingpong_context *ctx, int port, ibv_mtu_t mtu, int sl, const struct pingpong_dest *my_dest, const struct pingpong_dest *dest, int sgid_idx) { @@ -229,7 +229,7 @@ out: } static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx, - int ib_port, enum ibv_mtu mtu, + int ib_port, ibv_mtu_t mtu, int port, int sl, const struct pingpong_dest *my_dest, int sgid_idx) @@ -620,7 +620,7 @@ int main(int argc, char *argv[]) int port = 18515; int ib_port = 1; int size = 4096; - enum ibv_mtu mtu = IBV_MTU_1024; + ibv_mtu_t mtu = num_to_ibv_mtu(1024); int num_qp = 16; int rx_depth = 500; int iters = 1000; @@ -685,7 +685,7 @@ int main(int argc, char *argv[]) break; case 'm': - mtu = pp_mtu_to_enum(strtol(optarg, NULL, 0)); + mtu = num_to_ibv_mtu(strtol(optarg, NULL, 0)); if (mtu < 0) { usage(argv[0]); return 1; diff --git a/examples/uc_pingpong.c b/examples/uc_pingpong.c index 52c6c28..bafc2a6 100644 --- a/examples/uc_pingpong.c +++ b/examples/uc_pingpong.c @@ -78,7 +78,7 @@ struct pingpong_dest { }; static int pp_connect_ctx(struct pingpong_context *ctx, int port, int my_psn, - enum ibv_mtu mtu, int sl, + ibv_mtu_t mtu, int sl, struct pingpong_dest *dest, int sgid_idx) { struct ibv_qp_attr attr = { @@ -197,7 +197,7 @@ out: } static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx, - int ib_port, enum ibv_mtu mtu, + int ib_port, ibv_mtu_t mtu, int port, int sl, const struct pingpong_dest *my_dest, int sgid_idx) @@ -535,7 +535,7 @@ int main(int argc, char *argv[]) int port = 18515; int ib_port = 1; int size = 4096; - enum ibv_mtu mtu = IBV_MTU_1024; + ibv_mtu_t mtu = num_to_ibv_mtu(1024); int rx_depth = 500; int iters = 1000; int use_event = 0; @@ -596,7 +596,7 @@ int main(int argc, char *argv[]) break; case 'm': - mtu = pp_mtu_to_enum(strtol(optarg, NULL, 0)); + mtu = num_to_ibv_mtu(strtol(optarg, NULL, 0)); if (mtu < 0) { usage(argv[0]); return 1; diff --git a/examples/ud_pingpong.c b/examples/ud_pingpong.c index 21c551d..5a0656f 100644 --- a/examples/ud_pingpong.c +++ b/examples/ud_pingpong.c @@ -332,7 +332,7 @@ static struct pingpong_context *pp_init_ctx(struct ibv_device *ib_dev, int size, fprintf(stderr, "Unable to query port info for port %d\n", port); goto clean_device; } - mtu = 1 << (port_info.active_mtu + 7); + mtu = ibv_mtu_to_num(port_info.active_mtu); if (size > mtu) { fprintf(stderr, "Requested size larger than port MTU (%d)\n", mtu); goto clean_device; diff --git a/include/infiniband/verbs.h b/include/infiniband/verbs.h index 4b1ab57..2108629 100644 --- a/include/infiniband/verbs.h +++ b/include/infiniband/verbs.h @@ -144,6 +144,10 @@ struct ibv_device_attr { uint8_t phys_port_cnt; }; +/* + * Symbolic enum names for MTU values are preserved for backwards + * compatibility. + */ enum ibv_mtu { IBV_MTU_256 = 1, IBV_MTU_512 = 2, @@ -152,6 +156,14 @@ enum ibv_mtu { IBV_MTU_4096 = 5 }; +/* + * ibv_mtu_t is an encoded integer type that represents an MTU value. + * If the value is <= IBV_MTU_4096, it is treated as one of the + * IBV_MTU_* enum values. Otherwise, it is treated as its integer + * value. + */ +typedef int ibv_mtu_t; + enum ibv_port_state { IBV_PORT_NOP = 0, IBV_PORT_DOWN = 1, @@ -169,8 +181,8 @@ enum { struct ibv_port_attr { enum ibv_port_state state; - enum ibv_mtu max_mtu; - enum ibv_mtu active_mtu; + ibv_mtu_t max_mtu; + ibv_mtu_t active_mtu; int gid_tbl_len; uint32_t port_cap_flags; uint32_t max_msg_sz; @@ -485,7 +497,7 @@ enum ibv_mig_state { struct ibv_qp_attr { enum ibv_qp_state qp_state; enum ibv_qp_state cur_qp_state; - enum ibv_mtu path_mtu; + ibv_mtu_t path_mtu; enum ibv_mig_state path_mig_state; uint32_t qkey; uint32_t rq_psn; @@ -1138,6 +1150,43 @@ const char *ibv_port_state_str(enum ibv_port_state port_state); */ const char *ibv_event_type_str(enum ibv_event_type event); +/** + * num_to_ibv_mtu - Convert an integer to its corresponding encoded + * ibv_mtu_t value. If an integer value corresponding to an IBV_MTU_* + * enum value is passed, return the enum value (e.g., 1024 -> + * IBV_MTU_1024). Otherwise, just return the value (e.g., 1500 -> + * 1500). + */ +static inline ibv_mtu_t num_to_ibv_mtu(int num) +{ + switch (num) { + case 256: return IBV_MTU_256; + case 512: return IBV_MTU_512; + case 1024: return IBV_MTU_1024; + case 2048: return IBV_MTU_2048; + case 4096: return IBV_MTU_4096; + default: return num; + } +} + +/** + * ibv_mtu_to_num - Convert an encoded ibv_mtu_t value to its + * corresponding integer value. If an enum ibv_mtu value is passed, + * return its integer value (e.g., IBV_MTU_1024 -> 1024). Otherwise, + * just return the value (e.g., 1500 -> 1500). + */ +static inline int ibv_mtu_to_num(ibv_mtu_t mtu) +{ + switch (mtu) { + case IBV_MTU_256: return 256; + case IBV_MTU_512: return 512; + case IBV_MTU_1024: return 1024; + case IBV_MTU_2048: return 2048; + case IBV_MTU_4096: return 4096; + default: return mtu; + } +} + END_C_DECLS # undef __attribute_const diff --git a/man/ibv_modify_qp.3 b/man/ibv_modify_qp.3 index cb3faaa..26dfcf3 100644 --- a/man/ibv_modify_qp.3 +++ b/man/ibv_modify_qp.3 @@ -25,7 +25,7 @@ struct ibv_qp_attr { .in +8 enum ibv_qp_state qp_state; /* Move the QP to this state */ enum ibv_qp_state cur_qp_state; /* Assume this is the current QP state */ -enum ibv_mtu path_mtu; /* Path MTU (valid only for RC/UC QPs) */ +ibv_mtu_t path_mtu; /* Path MTU (valid only for RC/UC QPs) */ enum ibv_mig_state path_mig_state; /* Path migration state (valid if HCA supports APM) */ uint32_t qkey; /* Q_Key for the QP (valid only for UD QPs) */ uint32_t rq_psn; /* PSN for receive queue (valid only for RC/UC QPs) */ diff --git a/man/ibv_mtu_to_num.3 b/man/ibv_mtu_to_num.3 new file mode 100644 index 0000000..5603efa --- /dev/null +++ b/man/ibv_mtu_to_num.3 @@ -0,0 +1,67 @@ +.\" -*- nroff -*- +.\" +.TH IBV_MTU_TO_NUM 3 2013-06-20 libibverbs "Libibverbs Programmer's Manual" +.SH "NAME" +.nf +ibv_mtu_to_num \- convert encoded MTU value to integer +.sp +num_to_ibv_mtu \- convert integer to encoded MTU value +.SH "SYNOPSIS" +.nf +.B #include <infiniband/verbs.h> +.sp +.BI "int ibv_mtu_to_num(ibv_mtu_t " "mtu" "); +.sp +.BI "ibv_mtu_t num_to_ibv_mtu(int " "num" "); +.fi +.SH "DESCRIPTION" +.PP +The +.I ibv_mtu_t +type is an encoded integer used to represent MTU values in order to +preserve backwards compatibility. When the value of an +.I ibv_mtu_t +variable is <= +.BR IBV_MTU_4096\fR, +it is treated as the corresponding +.B IBV_MTU_* +enum value. Otherwise, it is treated as its integer value. +.PP +MTU values less than the value of the enum +.B IBV_MTU_4096 +(i.e., 5) cannot be represented. +.PP +.B ibv_mtu_to_num() +converts the encoded MTU value +.I mtu +to a plain integer value. For example, if +.I mtu +is +.BR IBV_MTU_1024\fR, +the value 1024 will be returned. Likewise, if +.I mtu +is 1500, then 1500 will be returned. +.PP +.B num_to_ibv_mtu() +converts the integer +.I num +to its corresponding encoded +.I ibv_mtu_t +value. For example, if +.I num +is 1024, then +.B IBV_MTU_1024 +will be returned. Likewise, if +.I num +is 1500, then 1500 will be returned. +.SH "RETURN VALUE" +.B ibv_mtu_to_num() +returns an integer MTU value. +.PP +.B num_to_ibv_mtu() +returns an encoded MTU value. +.SH "SEE ALSO" +.BR ibv_query_port (3) +.SH "AUTHORS" +.TP +Jeff Squyres <jsquyres@cisco.com> diff --git a/man/ibv_query_port.3 b/man/ibv_query_port.3 index 9bedd90..3700a5e 100644 --- a/man/ibv_query_port.3 +++ b/man/ibv_query_port.3 @@ -26,8 +26,8 @@ is an ibv_port_attr struct, as defined in <infiniband/verbs.h>. struct ibv_port_attr { .in +8 enum ibv_port_state state; /* Logical port state */ -enum ibv_mtu max_mtu; /* Max MTU supported by port */ -enum ibv_mtu active_mtu; /* Actual MTU */ +ibv_mtu_t max_mtu; /* Max MTU supported by port */ +ibv_mtu_t active_mtu; /* Actual MTU */ int gid_tbl_len; /* Length of source GID table */ uint32_t port_cap_flags; /* Port capabilities */ uint32_t max_msg_sz; /* Maximum message size */ diff --git a/man/ibv_query_qp.3 b/man/ibv_query_qp.3 index 3893ec8..ad7d9d5 100644 --- a/man/ibv_query_qp.3 +++ b/man/ibv_query_qp.3 @@ -30,7 +30,7 @@ struct ibv_qp_attr { .in +8 enum ibv_qp_state qp_state; /* Current QP state */ enum ibv_qp_state cur_qp_state; /* Current QP state - irrelevant for ibv_query_qp */ -enum ibv_mtu path_mtu; /* Path MTU (valid only for RC/UC QPs) */ +ibv_mtu_t path_mtu; /* Path MTU (valid only for RC/UC QPs) */ enum ibv_mig_state path_mig_state; /* Path migration state (valid if HCA supports APM) */ uint32_t qkey; /* Q_Key of the QP (valid only for UD QPs) */ uint32_t rq_psn; /* PSN for receive queue (valid only for RC/UC QPs) */
Keep IBV_MTU_* enums values as they are, but pass MTU values around as int's. This is an ABI-compatible change; legacy applications will use the enum values, but newer applications can use an int for values that do not currently exist in the enum set (e.g., 1500, 9000). (if people like the idea of this patch, I will send the corresponding kernel patch) Signed-off-by: Jeff Squyres <jsquyres@cisco.com> --- Makefile.am | 3 ++- examples/devinfo.c | 20 +++----------- examples/pingpong.c | 12 --------- examples/pingpong.h | 1 - examples/rc_pingpong.c | 8 +++--- examples/srq_pingpong.c | 8 +++--- examples/uc_pingpong.c | 8 +++--- examples/ud_pingpong.c | 2 +- include/infiniband/verbs.h | 55 ++++++++++++++++++++++++++++++++++--- man/ibv_modify_qp.3 | 2 +- man/ibv_mtu_to_num.3 | 67 ++++++++++++++++++++++++++++++++++++++++++++++ man/ibv_query_port.3 | 4 +-- man/ibv_query_qp.3 | 2 +- 13 files changed, 142 insertions(+), 50 deletions(-) create mode 100644 man/ibv_mtu_to_num.3