On 29.06.2005 [21:22:18 +0300], Michael S. Tsirkin wrote:
> Quoting r. Libor Michalek <[EMAIL PROTECTED]>:
> > 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
_______________________________________________
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
