On 4/26/05, David Teigland <[email protected]> wrote:
> On Mon, Apr 25, 2005 at 09:42:21AM -0700, Nish Aravamudan wrote:
> > On 4/25/05, David Teigland <[email protected]> wrote:
<snip>
> > Rather than wrap an infinite loop in an infinite loop, since all you
> > really want the second loop for is a periodic gurantee of
> > recover_timer seconds, wouldn't it be easier to have a sof-timer set
> > up to go off in that amount of time, and have it's callback do an
> > unconditional wake-up on the local wait-queue then mod_timer the timer
> > back in? Or might there be other tasks sleeping on the same
> > wait-queue? At that point, since your code does not seem to care about
> > signals (uses interruptible version, but doesn't check for
> > signal_pending() return value from wait_event), you probably could
> > just use the stock version of wait_event(). The conditions would be
> > checked in wait_event() on the desired periodic basis, just as they
> > are now, but maybe slightly more efficiently (and cleaner code, IMO :)
> > ). If you do *need* interruptible, please put in a comment as to why.
>
> Don't need inter, wait_event_timeout should work. Let's see if I followed
> all that; are you suggesting:
>
> static void dlm_wait_timer_fn(unsigned long data)
> {
> struct dlm_ls *ls = (struct dlm_ls *) data;
You can do the mod_timer() here if you make the timer_list global
(dlm_timer or something).
mod_timer(&dlm_timer, jiffies + dlm_config.recover_timer * HZ;
> wake_up(&ls->ls_wait_general);
> }
>
> int dlm_wait_function(struct dlm_ls *ls, int (*testfn) (struct dlm_ls *ls))
> {
> struct timer_list timer;
> int error = 0;
>
> init_timer(&timer);
> timer.function = dlm_wait_timer_fn;
> timer.data = (long) ls;
do an
timer.expires = jiffies + (dlm_config.recover_timer * HZ);
add_timer(&dlm_timer);
before wait_event().
> for (;;) {
Should not be need anymore.
> mod_timer(&timer, jiffies + (dlm_config.recover_timer * HZ));
Neither should this.
> wait_event(ls->ls_wait_general,
> testfn(ls) || dlm_recovery_stopped(ls));
So, now, you get woken up every so often to check the conditions *in*
wait_event() and if either is set you'll return to this context.
Otherwise, you'll go back to sleep in wait_event() -- that's why you
shouldn't need the infinite loop here.
> if (timer_pending(&timer))
> del_timer(&timer);
Good, this makes sure that you won't have the callback happening again
:) Do you need the sync version?
> if (testfn(ls))
> break;
Since you don't modify error in this case, you won't need this anymore
> if (dlm_recovery_stopped(ls)) {
> error = -1;
> break;
> }
Shouldn't need the break anymore, since you there is no loop.
> }
> return error;
> }
>
> [Another thread should usually be calling wake_up().]
>
> This is actually how it was some months ago, but I thought the timers made
> it more complicated given that wait_event_timeout is available. Do you
> really think the timers are nicer if we don't use interruptible?
The reason I think timers are nicer is that you don't need to nest
wait_event*() in a loop (and really, it looks strange when you do).
Complicated or not, I think they are the "right thing" for this case.
> > > +int dlm_wait_status_low(struct dlm_ls *ls, unsigned int wait_status)
> > > +{
> > > + struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf;
> > > + int error = 0, nodeid = ls->ls_low_nodeid;
> > > +
> > > + for (;;) {
> > > + error = dlm_recovery_stopped(ls);
> > > + if (error)
> > > + goto out;
> > > +
> > > + error = dlm_rcom_status(ls, nodeid);
> > > + if (error)
> > > + break;
> > > +
> > > + if (rc->rc_result & wait_status)
> > > + break;
> > > + else {
> > > + set_current_state(TASK_INTERRUPTIBLE);
> > > + schedule_timeout(HZ >> 1);
> >
> > 500 ms is a long time! What's the justification? No comments?
> > Especially considering you will wake up on *every* signal. It's
> > unlikely you'll actually sleep for 500 ms. Also, please use
> > msleep_interruptible(), unless you expect to be woken by wait-queues
> > (I didn't have enough time to trace all the possible code paths :) )
> > -- in which case, make it explicit with comments, please.
>
> This is polling the status of a remote node. All I'm after here is a
> delay between the dlm_rcom_status() calls so that repeated status messages
> aren't flooding the network. We keep sending status messages until the
> remote node reports the status we want to see.
>
> When the remote node has _failed_ we're repeating these status requests
> hopelessly until the membership system detects the node failure and
> recovery is aborted (dlm_recovery_stopped) -- up to several seconds. When
> the remote node is just _slower_ at doing recovery, I'm guessing the
> difference is on average a second.
>
> Given that info, do you have a suggested delay? I've already switched to
> msleep.
Nope, that seems reasonable (500ms).
Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
