[openib-general] SDP_CONN_LOCK

[Michael S. Tsirkin]

Hi, Libor!
I am not against optimizations, I am just expressing the
general sentiment that optimizations and tuning are
hard to judge on a theoretical basis.

It is not obvious to me that draining cq on each socket unlock
is a win, even if it was a win on gen1.



Quoting r. Libor Michalek <libor at topspin.com>:
> Subject: Re: [openib-general] SDP_CONN_LOCK
> 
> On Fri, Feb 18, 2005 at 01:25:39AM +0200, Michael S. Tsirkin wrote:
> > Quoting r. Libor Michalek <libor at topspin.com>:
> > > On Fri, Feb 18, 2005 at 12:13:30AM +0200, Michael S. Tsirkin wrote:
> > > > Quoting r. Libor Michalek <libor at topspin.com>:
> > > > > On Thu, Feb 17, 2005 at 11:29:37PM +0200, Michael S. Tsirkin wrote:
> > > > > > Quoting r. Libor Michalek <libor at topspin.com>:
> > > > > > > On Thu, Feb 17, 2005, Michael S. Tsirkin wrote:
> > > > > > > > Hi, Libor!
> > > > > > > > Could you please explain what are the SDP_CONN_LOCK
> > > > > > > > and friends doing in SDP?
> > > > > > > > 
> > > > > > > > It seems they just implement exclusive access to socket,
> > > > > > > > but if so, why is a simple semaphore or mutex not used?
> > > > > > > 
> > > > > > >   They do implement exclusive access to the socket, but they
> > > > > > >   implement exclusive access from both process and irq
> > > > > > >   context, which is why a semaphore was not used. In
> > > > > > >   interrupt context SDP_CONN_LOCK_BH is used to lock the
> > > > > > >   connection, look in sdp_cq_event_handler() for it's use,
> > > > > > >   and in process context SDP_CONN_LOCK is used.
> > > > > > 
> > > > > > I dont really understand how it works.
> > > > > > When an interrupt arrives while users != 0, it seems you are
> > > > > > calling scheduler().  What is sdp_conn_internal_lock doing?
> > > > > > I understand it is to be called from interrupt context, but
> > > > > > how can it call scheduler() then?
> > > > > 
> > > > >   SDP_CONN_LOCK and SDP_CONN_UNLOCK are never called from
> > > > >   interrupt context, only from process context. The spinlock
> > > > >   conn->lock.slock is called in interrupt context and process
> > > > >   context before the 'users' variable is changed or read. If
> > > > >   users != 0, then the connection is in use by process
> > > > >   context. If the connection is in use then another lock
> > > > >   request in process context will sleep until users == 0, if
> > > > >   the connection is in use then a lock request in interrupt
> > > > >   context will mark the connection as having had an event and
> > > > >   return. When a process context lock holder releases the
> > > > >   lock, it checks if there has been an interrupt event, which
> > > > >   it then processes by draining and arming the CQ, and it
> > > > >   wakes any process context waiting for the lock.
> > > > 
> > > > Cant all this be done by a semaphore in process context, and 
> > > > down_trylock in interrupt context?
> > > > The "connection has an event" bit could be protected by a spinlock.
> > > 
> > >   The CQ draining happens inside of the spinlock. If the CQ drain
> > > happened in process context inside of a semaphore, then there is
> > > a race condition for the interrupt between checking the semaphore
> > > and setting the spinlock protected "connection event bit". The
> > > CQ drain and the "event bit" have to happen under the same lock
> > > to avoid the race. 
> > > 
> > >   If you move the CQ drain and the "event bit" into the same spinlock
> > > where do you put the semaphore? In process context the spin would
> > > need to be inside the semaphore lock, but in interrupt context the
> > > semaphore down_trylock and up are inside the spinlock. Now there would
> > > be a race between spin_unlock() followed by up() in process context
> > > and spin_lock() followed by down_trylock() in interrupt context.
> > > 
> > >    spin_unlock()
> > >                   spin_lock()
> > >                   down_trylock()
> > >    up()
> > 
>
> [...]
>
> > Anyway, this flow is just an optimisation - so I wander how much does 
> > it help, actually.
> 
>   No, it's not just an optimization. The drain in process context is a
> optimization, but having a lock protecting in both contexts is necessary.

Thats what I mean. Cant we simply drain the CQ from the event handler.
Events overhead is much lower with gen2 than gen1, and the advantage
with CQ drain upon event is that you always actually have a completion
to handle.

This means access to sdp buffers will have to be spinlock protected,
but on the other hand you get rid of the spinlock protected "user" flag.


>   Without the drain in process context you would need to handle the actual
> CQ event, but if the connection is locked in process context, what do
> you do with the event? You either wait for the lock to be released, or
> you process it to a point, and then queue it for further handling once
> the lock is released. Waiting for a lock release is not practical, and 
> processing to a point is what we are currently doing, just we are
> processing it the bare minimum, and leaving the rest for process
> context.

Isnt this what spin_lock_irq is for? It shall protect the data
which can be accessed from both and irq and a process.

>   Setting the event bit and processing of the CQ in process context was
> actually a big simplification which not surprisingly performed much
> better as well.
> 
> > My understanding this is a backport from gen1, but what was a useful
> > optimisation for gen1 may not be for gen2. gen2 event flow is much 
> > faster than gen1 - it is possible all the extra locking and
> > complications are not worth it with mthca.
> 
>   I agree with the general sentiment. However, with mthca faster then the
> gen1 code, this code should help even more, since gen2 would be able to
> generate more interrupts a second, if we let it. At the time this has
> added, it resulted in a 2/3 reduction in interrupts per second. 

Since event are dispatched faster, the overhead of extra locking,
and the overhead of polling the cq when you dont actually know whether
it has a completion, is larger.

>   Next, in this portion of the code I was actually going to try using a
> single CQ instead of two to see if we get an additional improvement as a
> result of fewer interrupts and simpler code.
> 
> 
> -Libor

I think this is likely to help.

-- 
MST - Michael S. Tsirkin