[Openib-windows] Re: Complib changes to support OpenSM in OpenIB

[Eitan Zahavi]

Hi Fab,

Please the my responses regarding OpenSM changes inline:

Fab Tillier wrote:
> 
>>-----Original Message-----
>>From: Yossi Leybovich [mailto:sleybo at mellanox.co.il]
>>Sent: Thursday, August 18, 2005 4:32 AM
>>
>>1. cl_timers:
>>        a.
>>        we tried to call cl_timer_start from within the timer call back, and
>>the thread enter to deadlock . We notice that cl_timer_star call   to
>>cl_timer_stop which call to DeleteTimerQueueTimer with INVALI_HANDLE which
>>cause it to wait to all the callbacks to finish.
>>        Msdn:"...If this parameter is INVALID_HANDLE_VALUE, the function waits
>>for the timer callback function to complete before returning.    " So if we
>>call it within the timer callback we cause deadlock.
>>
>>        The kernel implementation of cl_timer_start does not call
>>cl_timer_stop why ?( I am not sure that in the Kernel one can be sure that
>>he want get more DPCs)
> 
> 
> In the kernel, the timer is implicitly cancelled if it is already in the queue
> it when it is re-queued.  See the docs for KeSetTimer.
> 
> In user-mode, we want to make sure that repeated calls to cl_timer_start don't
> result in multiple callbacks, so we explicitly cancel the timer if it exists.
> 
> 
>>        Solution to this problem is to add cl_timer_strat_cb function that
>>wont call to timer stop and be able to call
>>      from within the callback
> 
> 
> Let's try to find a better solution.  Are you always resetting the event from
> the callback?  Are you really looking for a periodic timer here, rather than a
> one-shot timer?  If so, it would make sense to expose periodic timers, rather
> than forcing the user to set a one-shot timer repeatedly.
In our (OpenSM) use the callback has to decide if a new event has to be 
scheduled or not. The time to the next event might be variable too. So 
making it a "repeating" timer (generating a series of events) is not 
really the required behavior.
I wonder what is so wrong with adding an option for the timer or even a 
different callback such that it will not prevent us from obtaining the 
required functionality.

> 
> 
> 
> 
>>2. cl_is_debug function - I send you patch for that , currently Linux version
>>of OpenSM use it so we need it in our complib as well
>>        I don't think its too risky
> 
> 
> Runtime checks for linkage errors don't belong, especially in Windows where we
> have distinct binary names for debug and release versions of the same component.
> If this is needed for Linux, put it in a #ifdef LINUX or something similar.
> Long term, I suggest someone fix the Linux side of things to prevent such
> linkage errors.
I agree that we should fix the LINUX side. But as things tend to take 
long time to change in the windows area so they take time in the LINUX 
area. So for now we simply will make the windows main.c little more 
different from the LINUX main.c by removing the section that checks the 
    "debug"-ness of the used libraries.
> 
> 
>>3. cl_qlock_pool.c - as Eitan said we need it in OpenSM for know and he will
>>change the code to work with external locks
> 
> 
> As I said before, I'd like to see this file stay in OpenSM.  If OpenSM moves
> away from using it, it can then be removed.  I don't want to add it to complib
> because I don't want any other ULPs to use it.  Further, the implementation
> should be entirely inline functions since there's very little code there.
Moving files around means changing "include" directives.
It is not only a matter of the directory the file is part of.
Regarding the usage of inline functions - I do not see how it matters.
It is just un-needed more work for me. Is there a simple rule you use 
for what has to be inline and what not?
If the code was all implemented in the header file will you agree to 
place it into the complib include directory?
>  
> 
  > As long as the OpenSM ib_types doesn't create linkage issues, there 
shouldn't be
> a problem here.  That means that the OpenSM version of ib_types.h needs to
> contain only static inline functions.
That is exactly the case. The only exception is ib_copy_ca_attr which is 
   a prototype that is actually never used and implemented. We can 
remove it.
I used the following to get that:
awk '/^[^         ][^      ]*[    ]*[(]/{print p,$0};{p=$0}' ib_types.h 
| grep -v "static inline"
> 
> - Fab