Messages in this thread | | | Subject | Re: [PATCH 4/6 OPTION-A version 3] completion: Add new wait_for_completion_timeout_state | From | Peter Zijlstra <> | Date | Tue, 27 Mar 2012 10:11:59 +0200 |
| |
On Mon, 2012-03-26 at 19:33 -0700, Boaz Harrosh wrote: > +int __sched > +wait_for_completion_timeout_state(struct completion *x, > + unsigned long timeout, int state) > +{ > + long t; > + int ret; > + > + if (!timeout) > + timeout = MAX_SCHEDULE_TIMEOUT; > + > + switch (state) { > + default: > + WARN_ON_ONCE(1); > + /* fall through */ > + case 0: > + state = TASK_UNINTERRUPTIBLE; > + break; > + case TASK_KILLABLE: > + case TASK_INTERRUPTIBLE: > + break; > + }
Don't do this, just pass in TASK_UNINTERRUPTIBLE (or TASK_NORMAL) whatever you like/need.
Don't fudge it by over-loading TASK_RUNNING (aka. 0).
> + t = wait_for_common(x, timeout, state); > + if (likely(t > 0)) { > + ret = 0; > + } else { > + if (t < 0) > + ret = t; > + else > + ret = -ETIMEDOUT; > + } > + return ret;
Again, why wreck an entirely reasonable return code and break with the rest of the API.
> +} > +EXPORT_SYMBOL(wait_for_completion_timeout_state);
So I'm fine with adding wait_for_completion_timeout_state(), but make it look and smell like wait_for_completion_timeout() and use a proper state, like wake_up_state().
IOW:
unsigned long __sched wait_for_completion_timeout_state(struct completion *x, unsigned long timeout, unsigned int state) { return wait_for_common(x, timeout, state); } EXPORT_SYMBOL(wait_for_completion_timeout_state);
| |