Hello!
hub.c in 2.4.0-test10 and above contains hardcoded HZ value,
which is wrong. Here is the patch:
--- drivers/usb/hub.c.orig Fri Nov 17 12:51:34 2000
+++ drivers/usb/hub.c Fri Nov 17 12:51:59 2000
@@ -813,7 +813,7 @@
ret = kill_proc(khubd_pid, SIGTERM, 1);
if (!ret) {
/* Wait 10 seconds */
- int count = 10 * 100;
+ int count = 10 * HZ;
while (khubd_running && --count) {
current->state = TASK_INTERRUPTIBLE;
Bye,
Oleg
On Fri, Nov 17, 2000, Oleg Drokin <[email protected]> wrote:
> hub.c in 2.4.0-test10 and above contains hardcoded HZ value,
> which is wrong. Here is the patch:
>
>
> --- drivers/usb/hub.c.orig Fri Nov 17 12:51:34 2000
> +++ drivers/usb/hub.c Fri Nov 17 12:51:59 2000
> @@ -813,7 +813,7 @@
> ret = kill_proc(khubd_pid, SIGTERM, 1);
> if (!ret) {
> /* Wait 10 seconds */
> - int count = 10 * 100;
> + int count = 10 * HZ;
>
> while (khubd_running && --count) {
> current->state = TASK_INTERRUPTIBLE;
We applied a slightly different patch which is would not remove the pages
out from under the thread, using semaphores instead.
This patch isn't needed anymore. Thanks anyway.
JE
[email protected] said:
> We applied a slightly different patch which is would not remove the
> pages out from under the thread, using semaphores instead.
> This patch isn't needed anymore. Thanks anyway.
Actually, the best fix is probably to get rid of the thread entirely and use
schedule_task to run usb_hub_events() instead.
--
dwmw2
On Tue, Nov 21, 2000, David Woodhouse <[email protected]> wrote:
>
> [email protected] said:
> > We applied a slightly different patch which is would not remove the
> > pages out from under the thread, using semaphores instead.
>
> > This patch isn't needed anymore. Thanks anyway.
>
> Actually, the best fix is probably to get rid of the thread entirely and use
> schedule_task to run usb_hub_events() instead.
That that possible? usb_hub_events can block for a long time. That is why
the kernel thread was needed. I'm not familiar with schedule_task enough
to know if it can be used.
JE
On Tue, 21 Nov 2000, Johannes Erdfelt wrote:
> That that possible? usb_hub_events can block for a long time. That is why
> the kernel thread was needed. I'm not familiar with schedule_task enough
> to know if it can be used.
Ah. How long? At first glance, it didn't look to me as if it would sleep
for long at all.
--
dwmw2
On Tue, Nov 21, 2000, David Woodhouse <[email protected]> wrote:
> On Tue, 21 Nov 2000, Johannes Erdfelt wrote:
>
> > That that possible? usb_hub_events can block for a long time. That is why
> > the kernel thread was needed. I'm not familiar with schedule_task enough
> > to know if it can be used.
>
> Ah. How long? At first glance, it didn't look to me as if it would sleep
> for long at all.
Multiple seconds in the worst case.
JE
[email protected] said:
> Multiple seconds in the worst case.
Well, I think the PCMCIA socket drivers would be happy with that. Depends
what akpm also added to the list of tasks, and whether Linus actually puts
that patch into test12.
Probably best to leave it for now and think about it in 2.5.
--
dwmw2
David Woodhouse wrote:
>
> [email protected] said:
> > Multiple seconds in the worst case.
>
> Well, I think the PCMCIA socket drivers would be happy with that. Depends
> what akpm also added to the list of tasks,
Nothing which sleeps for very long - mainly serial drivers which queue
a call to tty_hangup(), which immediately queues _another_ tq_scheduler
call to do_tty_hangup (Why? Heaven knows).
> and whether Linus actually puts
> that patch into test12.
tq_scheduler is a bug. You can't sleep, you can't call copy_*_user,
you can't call kmalloc non-atomically. But we do do these things.
Really the only reason for using tq_scheduler is so you can acquire
the tasklist_lock or the files_lock. schedule_task() is fine for that.
tq_scheduler must die.
[email protected] said:
> Nothing which sleeps for very long - mainly serial drivers which
> queue a call to tty_hangup(), which immediately queues _another_
> tq_scheduler call to do_tty_hangup (Why? Heaven knows).
Not so much worried about that. More about how sensitive they would be to
something _else_ causing the eventd thread to sleep for 'multiple seconds'
before getting round to doing what they asked.
I really don't want to have to start using multiple eventd threads before
2.5, if at all. So I don't want to add the USB hub stuff unless the other
queued tasks will be happy with that.
--
dwmw2
David Woodhouse wrote:
>
> [email protected] said:
> > Nothing which sleeps for very long - mainly serial drivers which
> > queue a call to tty_hangup(), which immediately queues _another_
> > tq_scheduler call to do_tty_hangup (Why? Heaven knows).
>
> Not so much worried about that. More about how sensitive they would be to
> something _else_ causing the eventd thread to sleep for 'multiple seconds'
> before getting round to doing what they asked.
Ah. No, I don't think it would be polite to cause TTY hangup processing
to be deferred for this long. I'd suggest that the policy be "scheduled
tasks can't sleep". I guess kmalloc(GFP_KERNEL) is acceptable because
the system is already running like a dog if this sleeps.
> I really don't want to have to start using multiple eventd threads before
> 2.5, if at all. So I don't want to add the USB hub stuff unless the other
> queued tasks will be happy with that.
Some of these requirements could also be satisfied with a combination of
a timer_list and a tq_struct. When the timer fires, feed the tq_struct
into schedule_task.
Easy, except for the problem of killing the damn things off reliably. That
would require a bit of infrastructure. But it's the right thing for
netdriver media polling functions, for example.
[email protected] said:
> Ah. No, I don't think it would be polite to cause TTY hangup
> processing to be deferred for this long. I'd suggest that the policy
> be "scheduled tasks can't sleep". I guess kmalloc(GFP_KERNEL) is
> acceptable because the system is already running like a dog if this
> sleeps.
Oi! I specifically added that so I could queue tasks which can sleep.
Put a time limit on it if you must, but not one which prohibits the
existing usage by the PCMCIA socket drivers.
I'm beginning to think that I should have argued with Linus? when he told
me to make it a generic thing rather than calling it pcmcia_queue_task()
? "Blasphemy! He said it again!"
--
dwmw2