[openib-general] Re: [PATCH] (repost) sdp: replace mlock with get_user_pages

[Libor Michalek]

On Wed, May 18, 2005 at 01:14:07AM +0300, Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <libor at topspin.com>:
> > Subject: Re: [PATCH] (repost) sdp: replace mlock with get_user_pages
> > 
> > On Sun, May 15, 2005 at 11:05:03AM +0300, Michael S. Tsirkin wrote:
> > > Quoting r. Libor Michalek <libor at topspin.com>:
> > 
> > > > +	/*
> > > > +	 * valid result can be 0 or 1 for complete so 
> > > > +	 * we ignore the value.
> > > > +	 */
> > > > +	(void)aio_complete(iocb->req, value, 0);
> > > >  	
> > > >  	if (in_atomic() || irqs_disabled()) {
> > > >  		INIT_WORK(&iocb->completion, do_iocb_complete, (void *)iocb);
> > > 
> > > By the way, as was already discussed here, in_atomic() may not
> > > catch all cases where its illegal to sleep.
> > > If the iocb functions get passed the socket parameter,
> > > the iocb can use the conn->lock.users flag to figure out that its
> > > OK to sleep.
> > 
> >   The conn->lock.users flag is not appropriate since it can be set and
> > it's still OK to sleep. For example it is set in numerous places where
> > we call kmalloc with the GFP_KERNEL flag. The conn->lock.users flag is
> > only modified inside a spinlock with IRQs disabled, but it can be either
> > value outside of that spinlock.
> > 
> > -Libor
> > 
> 
> SDP_CONN_LOCK_IRQ does not set the users flag.
> 
> If users is set this means that the socket was locked with
> sdp_conn_lock, which in turn means we are not in interrupt.
> Isnt that right?

  Now I see what you're driving at.

  Currently, the flag is still set when we process the backlog in 
sdp_conn_unlock() and then sdp_conn_internal_unlock() which is handled
under the spinlock, but there's no reason I can see not to clear it
ahead of sdp_conn_internal_unlock() instead of after, since it's all
under the same spinlock.  Same goes for sdp_conn_relock() it would
need to be cleared/reset before/after the drain all within the same
spinlock. At that point I see no reason not to use it to determine if
it is safe to sleep. Either that or make a second flag to avoid
overloading the "users" flag's meaning.

  However, I don't think having this knowledge in iocb_complete()
fixes the race condition I described. In the race I saw the iocb_complete()
was called correctly inside of sdp_conn_unlock() and then a following
aio write finished synchronously.

-Libor