<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=US-ASCII">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: [Openib-windows] Re: Complib changes to support OpenSM in OpenIB</TITLE>
</HEAD>
<BODY>
<P><FONT SIZE=2>Hi Fab,</FONT>
</P>
<P><FONT SIZE=2>Can you please explain in more detail what is wrong with the cl_qlockpool.c ? </FONT>
<BR><FONT SIZE=2>It seems to me like a very common programming way that there is a container that is single threaded, and in order to make it "multi-threaded save" one adds a lock around it.</FONT></P>
<P><FONT SIZE=2>I'm not saying that it the ideal solution, but it is very common and probably "good enough" for almost all implementations. </FONT></P>
<P><FONT SIZE=2>Thanks</FONT>
<BR><FONT SIZE=2>Tzachi</FONT>
</P>
<P><FONT SIZE=2>>-----Original Message-----</FONT>
<BR><FONT SIZE=2>>From: Eitan Zahavi [<A HREF="mailto:eitan@mellanox.co.il">mailto:eitan@mellanox.co.il</A>]</FONT>
<BR><FONT SIZE=2>>Sent: Wednesday, August 24, 2005 9:09 AM</FONT>
<BR><FONT SIZE=2>>To: Fab Tillier</FONT>
<BR><FONT SIZE=2>>Cc: Aviram Gutman; openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>>Subject: [Openib-windows] Re: Complib changes to support OpenSM in OpenIB</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>Fab Tillier wrote:</FONT>
<BR><FONT SIZE=2>>>>From: Eitan Zahavi [<A HREF="mailto:eitan@mellanox.co.il">mailto:eitan@mellanox.co.il</A>]</FONT>
<BR><FONT SIZE=2>>>>Sent: Tuesday, August 23, 2005 1:15 AM</FONT>
<BR><FONT SIZE=2>>>></FONT>
<BR><FONT SIZE=2>>>>Hi Fab,</FONT>
<BR><FONT SIZE=2>>>></FONT>
<BR><FONT SIZE=2>>>>Please the my responses regarding OpenSM changes inline:</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> Sure, see below, and thank you for sending in plain text - it makes it</FONT>
<BR><FONT SIZE=2>>much</FONT>
<BR><FONT SIZE=2>>> easier for me to reply.</FONT>
<BR><FONT SIZE=2>>No problem. I got used to use two mailers...</FONT>
<BR><FONT SIZE=2>>Not very productive but arguing with the rest of the LINUX developers is</FONT>
<BR><FONT SIZE=2>>not very productive either.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>>>Fab Tillier wrote:</FONT>
<BR><FONT SIZE=2>>>></FONT>
<BR><FONT SIZE=2>>> I think we can do something a little smarter in the implementation</FONT>
<BR><FONT SIZE=2>>without</FONT>
<BR><FONT SIZE=2>>> adding a new API - if we record what thread context we are in before</FONT>
<BR><FONT SIZE=2>>invoking</FONT>
<BR><FONT SIZE=2>>> the user callback (with GetCurrentThreadId). We can then check to see if</FONT>
<BR><FONT SIZE=2>>we are</FONT>
<BR><FONT SIZE=2>>> in the same thread when we invoke start, and not invoke stop at all. I'm</FONT>
<BR><FONT SIZE=2>>at IDF</FONT>
<BR><FONT SIZE=2>>> until Thursday, but I'll try to get something coded up so you can try it.</FONT>
<BR><FONT SIZE=2>>If</FONT>
<BR><FONT SIZE=2>>> you code it yourselves before I get back in the office, please send the</FONT>
<BR><FONT SIZE=2>>patch so</FONT>
<BR><FONT SIZE=2>>> I don't duplicate the effort.</FONT>
<BR><FONT SIZE=2>>This is a nice idea. We can wait for your return to the office. So we</FONT>
<BR><FONT SIZE=2>>will wait for your patch.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>>>I agree that we should fix the LINUX side. But as things tend to take</FONT>
<BR><FONT SIZE=2>>>>long time to change in the windows area so they take time in the LINUX</FONT>
<BR><FONT SIZE=2>>>>area. So for now we simply will make the windows main.c little more</FONT>
<BR><FONT SIZE=2>>>>different from the LINUX main.c by removing the section that checks the</FONT>
<BR><FONT SIZE=2>>>> "debug"-ness of the used libraries.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> Things do move slower than I wish - I wasn't saying fix the Linux side</FONT>
<BR><FONT SIZE=2>>now,</FONT>
<BR><FONT SIZE=2>>> rather just put a #ifdef for the check to only have it in Linux builds,</FONT>
<BR><FONT SIZE=2>>and add</FONT>
<BR><FONT SIZE=2>>> an item to the todo list for Linux.</FONT>
<BR><FONT SIZE=2>>As I said we are using a different main.c anyway.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>>>>>3. cl_qlock_pool.c - as Eitan said we need it in OpenSM for know and he</FONT>
<BR><FONT SIZE=2>>will</FONT>
<BR><FONT SIZE=2>>>>>>change the code to work with external locks</FONT>
<BR><FONT SIZE=2>>>>></FONT>
<BR><FONT SIZE=2>>>>>As I said before, I'd like to see this file stay in OpenSM. If OpenSM</FONT>
<BR><FONT SIZE=2>>moves</FONT>
<BR><FONT SIZE=2>>>>>away from using it, it can then be removed. I don't want to add it to</FONT>
<BR><FONT SIZE=2>>>>>complib</FONT>
<BR><FONT SIZE=2>>>>>because I don't want any other ULPs to use it. Further, the</FONT>
<BR><FONT SIZE=2>>implementation</FONT>
<BR><FONT SIZE=2>>>>>should be entirely inline functions since there's very little code</FONT>
<BR><FONT SIZE=2>>there.</FONT>
<BR><FONT SIZE=2>>>></FONT>
<BR><FONT SIZE=2>>>>Moving files around means changing "include" directives.</FONT>
<BR><FONT SIZE=2>>>>It is not only a matter of the directory the file is part of.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> You'll be moving files anyway, as well as updating include directives. I</FONT>
<BR><FONT SIZE=2>>don't</FONT>
<BR><FONT SIZE=2>>> think qlockpool has much value, and don't want to encourage its use. You</FONT>
<BR><FONT SIZE=2>>> currently have a complib subdirectory to osm. Keep that directory and</FONT>
<BR><FONT SIZE=2>>put the</FONT>
<BR><FONT SIZE=2>>> qlockpool file in there. The same applies to the cl_event_wheel thing.</FONT>
<BR><FONT SIZE=2>>OK.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>>>That is exactly the case. The only exception is ib_copy_ca_attr which is</FONT>
<BR><FONT SIZE=2>>>>a prototype that is actually never used and implemented.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> You mean it isn't used or implemented in OSM, right?</FONT>
<BR><FONT SIZE=2>>I mean ib_types.h does not declare any non static inline functions but</FONT>
<BR><FONT SIZE=2>>the ib_copy_ca_attr which can be removed.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> Also, there was an issue about catching signals, and you were going to</FONT>
<BR><FONT SIZE=2>>look at</FONT>
<BR><FONT SIZE=2>>> using the SetConsoleCtrlHandler function, or even better eliminating that</FONT>
<BR><FONT SIZE=2>>> functionality since the underlying access layers should handle the app</FONT>
<BR><FONT SIZE=2>>exiting.</FONT>
<BR><FONT SIZE=2>>> What is the status of this?</FONT>
<BR><FONT SIZE=2>>I think we could do that - I.e. do not catch the kill.</FONT>
<BR><FONT SIZE=2>>I need to double check on the LINUX front. In the past adding the signal</FONT>
<BR><FONT SIZE=2>>catch helped preventing kernel resources leak and even kernel oops.</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> Thanks,</FONT>
<BR><FONT SIZE=2>>></FONT>
<BR><FONT SIZE=2>>> - Fab</FONT>
<BR><FONT SIZE=2>></FONT>
<BR><FONT SIZE=2>>_______________________________________________</FONT>
<BR><FONT SIZE=2>>openib-windows mailing list</FONT>
<BR><FONT SIZE=2>>openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>><A HREF="http://openib.org/mailman/listinfo/openib-windows" TARGET="_blank">http://openib.org/mailman/listinfo/openib-windows</A></FONT>
</P>
</BODY>
</HTML>