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

[Michael S. Tsirkin]

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?

-- 
MST