[PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake_up
Rafał Miłecki
---
We try to implement some PM in radeon KMS and we need to sync with VLBANK for
reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
timer handler just before reclocking. Then our IRQ handler calls wake_up and we
continue reclocking.
As you see our sleeping is condition-less, we just wait for waking up queue.
We hope this waking will happen from IRQ handler, but for less-happy case we
also use some timeout (this will probably cause some single corruption, but
we can live with it).
Following macro is soemthing that seems to work fine for us, but instead
introducing this to radeon KMS only, I'd like to propose adding this to whole
wait.h. Do you this it's something we should place there? Can someone take this
patch for me? Or maybe you find this rather useless and we should keep this
marco locally?
---
include/linux/wait.h | 25 +++++++++++++++++++++++++
1 files changed, 25 insertions(+), 0 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index a48e16b..998475b 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -332,6 +332,31 @@ do { \
__ret; \
})
+/**
+ * wait_interruptible_timeout - sleep until a waitqueue is woken up
+ * @wq: the waitqueue to wait on
+ * @timeout: timeout, in jiffies
+ *
+ * The process is put to sleep (TASK_INTERRUPTIBLE) until the waitqueue
+ * @wq is woken up. It can be done manually with wake_up or will happen
+ * if timeout elapses.
+ *
+ * The function returns 0 if the @timeout elapsed, remaining jiffies
+ * if workqueue was waken up earlier.
+ */
+#define wait_interruptible_timeout(wq, timeout) \
+({ \
+ long __ret = timeout; \
+ \
+ DEFINE_WAIT(__wait); \
+ prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \
+ if (!signal_pending(current)) \
+ __ret = schedule_timeout(__ret); \
+ finish_wait(&wq, &__wait); \
+ \
+ __ret; \
+})
+
#define __wait_event_interruptible_exclusive(wq, condition, ret) \
do { \
DEFINE_WAIT(__wait); \
--
1.6.4.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Thomas Hellstrom
/Thomas
Rafał Miłecki
<tho...@shipmail.org> napisał:
I guess this will wake up on every signal pending to driver's process.
I need to wake up using my own (VBLANK related) workqueue.
Is that right? Or maybe there is some hack/sth that will let me
achieve what I need?
--
Rafał
Rafał Miłecki
Can I interpret lack of objections as permission for committing that?
If so, by which tree should we get this patch mainline?
Dave: this patch is needed for radeon driver. Can we get this through
drm-2.6 maybe?
Rafał Miłecki
---------- Wiadomość przekazana dalej ----------
From: Rafał Miłecki <zaj...@gmail.com>
Date: 21 lutego 2010 15:10
Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro to
sleep (w. timeout) until wake_up
To: Linux Kernel Mailing List <linux-...@vger.kernel.org>,
dri-...@lists.sourceforge.net
CC: Rafał Miłecki <zaj...@gmail.com>
Thomas Gleixner
> Forwarding to ppl I could often notice in git log time.h
And how is this related to time.h ?
>
> ---------- Wiadomość przekazana dalej ----------
> From: Rafał Miłecki <zaj...@gmail.com>
> Date: 21 lutego 2010 15:10
> Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro to
> sleep (w. timeout) until wake_up
> To: Linux Kernel Mailing List <linux-...@vger.kernel.org>,
> dri-...@lists.sourceforge.net
> CC: Rafał Miłecki <zaj...@gmail.com>
>
>
> Signed-off-by: Rafał Miłecki <zaj...@gmail.com>
> ---
> We try to implement some PM in radeon KMS and we need to sync with VLBANK for
> reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
> timer handler just before reclocking. Then our IRQ handler calls wake_up and we
Sleeping in the timer handler ? In which context runs this timer handler ?
> continue reclocking.
>
> As you see our sleeping is condition-less, we just wait for waking up queue.
Your sleep is not condition-less at all. See below.
> We hope this waking will happen from IRQ handler, but for less-happy case we
> also use some timeout (this will probably cause some single corruption, but
> we can live with it).
>
> Following macro is soemthing that seems to work fine for us, but instead
> introducing this to radeon KMS only, I'd like to propose adding this to whole
> wait.h. Do you this it's something we should place there? Can someone take this
> patch for me? Or maybe you find this rather useless and we should keep this
> marco locally?
You better delete it right away. It's broken and racy.
If the interrupt happens right before you enqueue to the wake queue,
then you miss it. The old interruptible_sleep_on_timeout() was
replaced by the event wait functions which have a condition exaclty to
avoid that race.
And you have a condition: Did an interrupt happen?
Thanks,
tglx
Rafał Miłecki
<tg...@linutronix.de> napisał:
> On Fri, 26 Feb 2010, Rafał Miłecki wrote:
>
>> Forwarding to ppl I could often notice in git log time.h
>
> And how is this related to time.h ?
Ouch, time.h vs. wait.h. I'm sorry.
>> ---------- Wiadomość przekazana dalej ----------
>> From: Rafał Miłecki <zaj...@gmail.com>
>> Date: 21 lutego 2010 15:10
>> Subject: [PATCH][RFC] time: add wait_interruptible_timeout macro to
>> sleep (w. timeout) until wake_up
>> To: Linux Kernel Mailing List <linux-...@vger.kernel.org>,
>> dri-...@lists.sourceforge.net
>> CC: Rafał Miłecki <zaj...@gmail.com>
>>
>>
>> Signed-off-by: Rafał Miłecki <zaj...@gmail.com>
>> ---
>> We try to implement some PM in radeon KMS and we need to sync with VLBANK for
>> reclocking engine/memory. The easiest and cleanest way seems to be sleeping in
>> timer handler just before reclocking. Then our IRQ handler calls wake_up and we
>
> Sleeping in the timer handler ? In which context runs this timer handler ?
We have our struct delayed_work which we first init and then we use
"queue_delayed_work" to start this "timer". So it's not real-real
timer as struct timer_list.
So this is actually delayed_work handler. Sorry (again) for my bad naming.
Anyway in this handler we just take decision about (down|up)clocking,
we wait for VBLANK (to avoid display corrupted picture) and right
after it happens, we reclock engine (plus memory in future).
>> continue reclocking.
>>
>> As you see our sleeping is condition-less, we just wait for waking up queue.
>
> Your sleep is not condition-less at all. See below.
>
>> We hope this waking will happen from IRQ handler, but for less-happy case we
>> also use some timeout (this will probably cause some single corruption, but
>> we can live with it).
>>
>> Following macro is soemthing that seems to work fine for us, but instead
>> introducing this to radeon KMS only, I'd like to propose adding this to whole
>> wait.h. Do you this it's something we should place there? Can someone take this
>> patch for me? Or maybe you find this rather useless and we should keep this
>> marco locally?
>
> You better delete it right away. It's broken and racy.
>
> If the interrupt happens right before you enqueue to the wake queue,
> then you miss it. The old interruptible_sleep_on_timeout() was
> replaced by the event wait functions which have a condition exaclty to
> avoid that race.
Well, I'm completely fine with that. After taking decision about
reclocking I request hardware to start reporting VBLANK interrupts.
Then (without any hurry) I go into sleep and next VBLANK interrupt
wake me up. Right after waking up I reclock engine/memory and then
(without hurry) I tell hardware to stop reporting VBLANK interrupts.
I guess it can be AMD-GPU specific interrupts mechanism there, but
that's how it works. I can point responsible code in driver if you
wish.
> And you have a condition: Did an interrupt happen?
Yeah, I guess that's kind of condition. I meant that I don't use any
driver's variable as condition to stop sleeping.
Sorry again for my mistakes mentioned above.
--
Rafał
Andrew Morton
> +#define wait_interruptible_timeout(wq, timeout)
> \
> +({ \
> + long ret = timeout; \
> + \
> + DEFINE_WAIT(wait); \
> + prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE); \
> + if (!signal_pending(current)) \
> + ret = schedule_timeout(ret); \
> + finish_wait(&wq, &wait); \
> + \
> + ret; \
> +})
It's often a mistake to use signals in-kernel. Signals are more a
userspace thing and it's better to use the lower-level kernel-specific
messaging tools in-kernel. Bear in mind that userspace can
independently and asynchronously send, accept and block signals.
Can KMS use wait_event_interruptible_timeout()?
Rafał Miłecki
<ak...@linux-foundation.org> napisał:
> On Fri, 26 Feb 2010 11:38:59 +0100 Rafa Miecki <zaj...@gmail.com> wrote:
>
>> +#define wait_interruptible_timeout(wq, timeout)
>> \
>> +({ \
>> + long ret = timeout; \
>> + \
>> + DEFINE_WAIT(wait); \
>> + prepare_to_wait(&wq, &wait, TASK_INTERRUPTIBLE); \
>> + if (!signal_pending(current)) \
>> + ret = schedule_timeout(ret); \
>> + finish_wait(&wq, &wait); \
>> + \
>> + ret; \
>> +})
>
> It's often a mistake to use signals in-kernel. Signals are more a
> userspace thing and it's better to use the lower-level kernel-specific
> messaging tools in-kernel. Bear in mind that userspace can
> independently and asynchronously send, accept and block signals.
Can you point me to something kernel-level please?
> Can KMS use wait_event_interruptible_timeout()?
No. Please check definition of this:
#define wait_event_interruptible_timeout(wq, condition, timeout) \
({ \
long __ret = timeout; \
if (!(condition)) \
__wait_event_interruptible_timeout(wq, condition, __ret); \
__ret; \
})
It uses condition there, but that's not a big issue. We just need to
pass 0 (false) there and it will work so far.
But then check __wait_event_interruptible_timeout definition, please.
It goes into sleep and after waking up it checks for value returned by
schedule_timeout. That's what breaks our (needed by radeon) sleeping.
If timeout didn't expire it does into sleep again!
What we need is continue reclocking after waking up. If this has
happend before timeout expired, that means we was woken up by VBLANK
interrupt handler. We love that situation and we do not want to go
sleep again.
On the other hand we need to have some timeout in case VBLANK
interrupt won't come.
--
Rafał
Ville Syrjälä
Disabling the condition check doesn't make sense.
You could use a completion.
init_completion(vbl_irq);
enable_vbl_irq();
wait_for_completion(vbl_irq);
disable_vbl_irq();
and call complete(vbl_irq) in the interrupt handler.
The same would of course work with just some flag or counter
and a wait queue. Isn't there already a vbl counter that you could
compare in the condition?
--
Ville Syrjälä
syr...@sci.fi
http://www.sci.fi/~syrjala/
Linus Torvalds
On Fri, 26 Feb 2010, Rafał Miłecki wrote:
>
> Following macro is soemthing that seems to work fine for us, but instead
> introducing this to radeon KMS only, I'd like to propose adding this to whole
> wait.h. Do you this it's something we should place there? Can someone take this
> patch for me? Or maybe you find this rather useless and we should keep this
> marco locally?
This does not smell generic to me. In fact, it makes me personally think
you're doing something wrong in the first place, but maybe it's ok. But in
case it really is ok, I'd still not put it in a generic header file unless
you can point to other cases where it really makes sense to do this.
Linus
Rafał Miłecki
> Disabling the condition check doesn't make sense.
>
> You could use a completion.
>
> init_completion(vbl_irq);
> enable_vbl_irq();
> wait_for_completion(vbl_irq);
> disable_vbl_irq();
> and call complete(vbl_irq) in the interrupt handler.
>
> The same would of course work with just some flag or counter
> and a wait queue.
Ouch, I can see it gone bad already.
Firstly I simply just wanted to avoid condition in wait_event_*. It
looked unnecessary as I got interrupts (signals). So I started playing
with other solutions (like my wait_interruptible_timeout where I had
not full understanding of waking up) and finally started analyzing
over-complex things like completions.
I'll just use some one more variable and some more basic solution.
Thanks for help and sorry for taking your time. I hope to provide at
least some of you dynamic radeon PM in return :)
--
Rafał
Michel Dänzer
> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syr...@sci.fi> napisał:
> > Disabling the condition check doesn't make sense.
> >
> > You could use a completion.
> >
> > init_completion(vbl_irq);
> > enable_vbl_irq();
> > wait_for_completion(vbl_irq);
> > disable_vbl_irq();
> > and call complete(vbl_irq) in the interrupt handler.
> >
> > The same would of course work with just some flag or counter
> > and a wait queue.
>
> Ouch, I can see it gone bad already.
>
> Firstly I simply just wanted to avoid condition in wait_event_*. It
> looked unnecessary as I got interrupts (signals).
So this code runs in user process context? If so, it should return to
userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
movement may suffer.
If that's a problem, then maybe the code should run in a different
context, e.g. a tasklet or some kind of worker kernel thread.
--
Earthling Michel Dänzer | http://www.vmware.com
Libre software enthusiast | Debian, X and DRI developer
Rafał Miłecki
> On Sat, 2010-02-27 at 10:33 +0100, Rafał Miłecki wrote:
>> W dniu 26 lutego 2010 20:01 użytkownik Ville Syrjälä <syr...@sci.fi> napisał:
>> > Disabling the condition check doesn't make sense.
>> >
>> > You could use a completion.
>> >
>> > init_completion(vbl_irq);
>> > enable_vbl_irq();
>> > wait_for_completion(vbl_irq);
>> > disable_vbl_irq();
>> > and call complete(vbl_irq) in the interrupt handler.
>> >
>> > The same would of course work with just some flag or counter
>> > and a wait queue.
>>
>> Ouch, I can see it gone bad already.
>>
>> Firstly I simply just wanted to avoid condition in wait_event_*. It
>> looked unnecessary as I got interrupts (signals).
>
> So this code runs in user process context? If so, it should return to
> userspace ASAP on signal receipt, otherwise e.g. smoothness of X mouse
> movement may suffer.
>
> If that's a problem, then maybe the code should run in a different
> context, e.g. a tasklet or some kind of worker kernel thread.
It has nothing to do with userspace. Please see my previous description:
W dniu 26 lutego 2010 13:16 użytkownik Rafał Miłecki <zaj...@gmail.com> napisał:
> W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
>> Sleeping in the timer handler ? In which context runs this timer handler ?
>
> We have our struct delayed_work which we first init and then we use
> "queue_delayed_work" to start this "timer". So it's not real-real
> timer as struct timer_list.
>
> So this is actually delayed_work handler. Sorry (again) for my bad naming.
It's delayed_work.
--
Rafał