> Quoting Steve Wise <[EMAIL PROTECTED]>: > Subject: Re: [PATCH] The ibv_cmd_* create functions need to set the context. > > On Wed, 2007-01-31 at 12:24 +0200, Michael S. Tsirkin wrote: > > > Quoting Roland Dreier <[EMAIL PROTECTED]>: > > > Subject: Re: [PATCH] The ibv_cmd_* create functions need to set the > > > context. > > > > > > Thanks, applied to master and stable branches. > > > > Did you test it? > > This patch (8b3d225476c99ea29a68109a7d40e5ef353d4388) causes ibv_ud_pingpong > > to segfault on libmthca: libmthca never calls ibv_cmd_create_ah to context > > is now > > never set. > > > > > > I didn't test UD.
Well, when you touch the AH functions, UD is really the only way to test them. > > > Starting program: /usr/local/ofed/bin/ibv_ud_pingpong sw069 > > [Thread debugging using libthread_db enabled] > > [New Thread 47299578320592 (LWP 5085)] > > local address: LID 0x0002, QPN 0x090406, PSN 0x71bffb > > remote address: LID 0x0001, QPN 0x040406, PSN 0x92316a > > 4096000 bytes in 0.02 seconds = 1893.99 Mbit/sec > > 1000 iters in 0.02 seconds = 17.30 usec/iter > > > > Program received signal SIGSEGV, Segmentation fault. > > [Switching to Thread 47299578320592 (LWP 5085)] > > 0x00002b04ca3b7263 in __ibv_destroy_ah (ah=0x5050b0) at src/verbs.c:475 > > 475 return ah->context->ops.destroy_ah(ah); > > (gdb) p ah->context > > $1 = (struct ibv_context *) 0x0 > > > > I actually think this approach is a wrong one: context should be > > set in common code like ibv_create_ah, not in ibv_cmd_ which is > > a library function low level driver might or might not call. > > And certainly this kind of change does not seem appropriate for stable > > branch. > > > > I think the proper thing is for low level driver not to assume that > > fields such as contex are intialized until create functions have returned. > > Steve, pls fix your low level driver not to rely on this. > > > > The issue is that the provider lib calls ibv_cmd_create_blah to create > the object, then some failure happens (like a failure mmap()ing the > object's DMA area to the process). At this point the provider lib must > destroy this object that is created from the perspective of the ibv_cmd* > interface. The only way to do that is to call the ibv_cmd_destroy_blah > call, which needs the context field. For stable, in case of error, set the context in the provider lib then? > So I don't think solving this in the provider lib is the right thing to > do. At least for stable branch, this seams more sensible than the disruptive patch that was applied. Roland, what do you think? For master, maybe ibv_cmd destructors should get the context as a parameter? > > Roland, I have reverted this in OFED, please revert on master and stable. > > > > I think we should fix the bug introduced: set the context field in the > ibv_create_blah service if its not set after calling the provider > method. This is ugly as well, but at least it would work. -- MST _______________________________________________ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general