linux-kernel - Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep (w. timeout) until wake

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b170af451002260416i474617f5v2cc20e0d1c55def9@mail.gmail.com>
Date:	Fri, 26 Feb 2010 13:16:39 +0100
From:	Rafał Miłecki <zajec5@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	DRI <dri-devel@...ts.sourceforge.net>
Subject: Re: [PATCH][RFC] time: add wait_interruptible_timeout macro to sleep 
	(w. timeout) until wake_up

W dniu 26 lutego 2010 12:55 użytkownik Thomas Gleixner
<tglx@...utronix.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 <zajec5@...il.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-kernel@...r.kernel.org>,
>> dri-devel@...ts.sourceforge.net
>> CC: Rafał Miłecki <zajec5@...il.com>
>>
>>
>> Signed-off-by: Rafał Miłecki <zajec5@...il.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ł
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/