[openib-general] Re: [PATCH] sdp_inet: fix schedule_timeout() usage

[Nishanth Aravamudan]

On 29.06.2005 [21:22:18 +0300], Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <libor at topspin.com>:
> > Subject: Re: [PATCH] sdp_inet: fix schedule_timeout() usage
> > 
> > On Tue, Jun 28, 2005 at 01:48:55PM -0700, Nishanth Aravamudan wrote:
> > >
> > > Using schedule_timeout() without setting the state first is broken and
> > > causes schedule_timeout() to return immediately (effectively you call
> > > schedule() without changing your state and are thus going to run again).
> > > In each of these loops in sdp_inet.c involving schedule_timeout(), the
> > > first iteration is correct, but subsequent ones result in busy-wait. Add
> > > the appropriate set_current_state() call to fix the issue.
> > 
> > Nish,
> > 
> >   Thank you for the patch. I'm wondering if it would be better to just
> > move the existing call to set_current_state(TASK_INTERRUPTIBLE) from
> > outside the loop to inside at the begining, as below?
> > 
> > -Libor
> > 
> > Index: sdp_inet.c
> > ===================================================================
> > --- sdp_inet.c	(revision 2749)
> > +++ sdp_inet.c	(working copy)
> > @@ -317,10 +317,11 @@
> >  			timeout = sk->sk_lingertime;
> >  			
> >  			add_wait_queue(sk->sk_sleep, &wait);
> > -			set_current_state(TASK_INTERRUPTIBLE);
> >  
> >  			while (timeout > 0 &&
> >  			       !(SDP_ST_MASK_CLOSED & conn->state)) {
> > +
> > +				set_current_state(TASK_INTERRUPTIBLE);
> >  				sdp_conn_unlock(conn);
> >  				timeout = schedule_timeout(timeout);
> >  				sdp_conn_lock(conn);
> > @@ -554,10 +555,10 @@
> >  
> >  		DECLARE_WAITQUEUE(wait, current);
> >  		add_wait_queue(sk->sk_sleep, &wait);
> > -		set_current_state(TASK_INTERRUPTIBLE);
> >  
> >  		while (timeout > 0 && (conn->state & SDP_ST_MASK_CONNECT)) {
> >  
> > +			set_current_state(TASK_INTERRUPTIBLE);
> >  			sdp_conn_unlock(conn);
> >  			timeout = schedule_timeout(timeout);
> >  			sdp_conn_lock(conn);
> > @@ -710,11 +711,12 @@
> >  		if (!accept_conn) {
> >  			DECLARE_WAITQUEUE(wait, current);
> >  			add_wait_queue(listen_sk->sk_sleep, &wait);
> > -			set_current_state(TASK_INTERRUPTIBLE);
> >  
> >  			while (timeout > 0 &&
> >  			       listen_conn->state == SDP_CONN_ST_LISTEN &&
> >  			       !listen_conn->backlog_cnt) {
> > +
> > +				set_current_state(TASK_INTERRUPTIBLE);
> >  				sdp_conn_unlock(listen_conn);
> >  				timeout = schedule_timeout(timeout);
> >  				sdp_conn_lock(listen_conn);
> > 
> 
> Shouldnt we be using msleep_interruptible rather than playing with
> task states explicitly?

Not in these cases; msleep_{,interruptible}() cannot be used around
wait-queues (they both loop on the requested timeout, and thus ignore
all wake-up events (except signals in the latter case)).

I am verifying that no other places in the openib code might be able to
take advantage of these helpers, though.

Thanks,
Nish