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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
