hardcoded HZ in hub.c

hardcoded HZ in hub.c

[Oleg Drokin]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[Johannes Erdfelt]

On Fri, Nov 17, 2000, Oleg Drokin <green@ixcelerator.com> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[David Woodhouse]

jerdfelt@valinux.com 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[Johannes Erdfelt]

On Tue, Nov 21, 2000, David Woodhouse <dwmw2@infradead.org> wrote:
> 
> jerdfelt@valinux.com 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[David Woodhouse]

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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[Johannes Erdfelt]

On Tue, Nov 21, 2000, David Woodhouse <dwmw2@infradead.org> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[David Woodhouse]

johannes@erdfelt.com 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[Andrew Morton]

David Woodhouse wrote:
> 
> johannes@erdfelt.com 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[David Woodhouse]

andrewm@uow.edu.au 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[Andrew Morton]

David Woodhouse wrote:
> 
> andrewm@uow.edu.au 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: hardcoded HZ in hub.c

[David Woodhouse]

andrewm@uow.edu.au 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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/